Failed orientation sensor communication no longer hangs/reboots system #166
Conversation
|
@ba58smith - since you brought up the issue, and fixed it in the BME280 code, would you mind reviewing my orientation sensor fixes for the same problem? Thanks a bunch! |
|
It appears that your code is doing what your EXCELLENT description says. Since you've tested it, and it works as expected - please resolve the one TODO comment above, and I'll merge it. Nice work. I need to start describing my PR's as well as you do. |
|
@ba58smith - back to you. The TODO is DONE, and there are no adverse effects. Thanks for your kind words re: documentation. I've noticed over the years that my memory is short enough that when I return to something a few months later I start cussing my past self if I haven't written stuff down :-) That, plus my work in the field of medical device design, motivates me to try for thoroughness... |
|
Please remove the first three unrelated commits from the PR. You can squash the extra away by interactive rebasing (i.e. history editing), git rebase -i on the command line or (probably easier) by using some graphical user interface tool. If that proves difficult, you can clone a fresh copy of the repo and reapply the new changes. To avoid creating bogus merge commits, use rebasing instead of merging when pulling new changes (git pull --rebase on command line, the respective option in a GUI). |
|
@mairas - sorry, I've made a hash of this. I've tried rebasing using both command-line git and Github Desktop, but I can't seem to squash all the commits into one on my remote repository. Rebasing locally seems to work, but then when I try pushing that to my Github repository it complains that my current branch is behind its remote counterpart. The closest I could come was to revert the unwanted commits, but while that gets rid of the unwanted changes, it also adds yet another commit to the history. When you suggest "...clone a fresh copy of the repo and reapply the new changes" could you describe in more detail how I go about that? Is that the same as closing this PR and starting a new one? |
|
@BjarneBitscrambler - I think what he means is for you to completely delete your local copy of the SensESP files - the ones that are your Git files. Don't just delete the files, delete the folder. Also, delete your .pio folder. under your PlatformIO SensESP project(s). Now, in your Git "root" directory (the directory in which all your Git projects live - for me it's Documents/GitHubProjects), do Then you need to create your git remotes - typically called "origin" and "upstream", which point to your fork of SensESP on GitHub, and to the main SensESP repo on GitHub. (The "origin" remote might be created by the "clone" command - I don't remember. Google how to set up "upstream" and "origin" remotes.) At this point, everything should be clean, and in sync with the main SensESP repo, and you can start making commits that shouldn't know anything about any previous commits. I think. Something like that, anyway. |
|
@BjarneBitscrambler Brian's advice is spot on if you want to start clean; however, if you managed to locally rebase, that's great! Now, when you try to push to the remote branch, yes, Git will complain because you'll be rewriting history. But when you're doing that on your own branch, that's perfectly fine! You just have to "force-push", that is, However, be warned though; I made a PR to remove the magnetometer code. That is temporary only; there were dependency issues due to conflicting library versions, aggravated by the coinciding PlatformIO version update. Once the dependency issues have been resolved, your code must be taken back in, of course. But while this is under progress, this PR should remain on hold. |
BjarneBitscrambler
force-pushed
the
master
branch
from
68c0a44
to
a07dcb8
Compare
|
OK - thanks! The advice from @mairas to force the push was successful, but I appreciate the explanation of the how to do a clean restart from @ba58smith. The extraneous commits have now been 'forgotten' and the two sensor files seem to be in the desired state. Never mind about the following problem - I did another rebase to remove Brian's commit, and it seems fixed now. Rebasing and force-pushing are pretty powerful tools... :-) |
|
@mairas - no worries about holding off on the orientation sensor until after the library dependencies are sorted out. |
BjarneBitscrambler
force-pushed
the
master
branch
from
a07dcb8
to
a4caab5
Compare
BjarneBitscrambler
force-pushed
the
master
branch
from
1dfd791
to
9d41716
Compare
-also renamed some variables to conform with project coding style -moved global instances of sensors from .cpp to inside class -fleshed out setupSensors() to make gyro setting appropriate to sampling rate
| #include <Adafruit_FXAS21002C.h> | ||
| #include <Adafruit_FXOS8700.h> | ||
|
|
||
| //#define AHRS_DEBUG_OUTPUT //output diagnostics from sensor read and filter | ||
| #define MIN_PRINT_INTERVAL_MS \ | ||
| (200) // when result printing is requested, it won't happen | ||
| // any more frequently than this, | ||
| // to avoid overloading the serial channel | ||
|
|
||
| // can the following be moved inside class? or into main.cpp? | ||
| Adafruit_FXOS8700 fxos = Adafruit_FXOS8700(0x8700A, 0x8700B); | ||
| Adafruit_FXAS21002C fxas = Adafruit_FXAS21002C(0x0021002C); |
There was a problem hiding this comment.
Moved the fxos and fxas objects inside class (see .h file) and instantiated in SensorNXP_FXOS8700_FXAS21002 constructor.
| // TODO: set ranges/sensitivity. For now use ICs' default values. | ||
| bool SensorNXP_FXOS8700_FXAS21002::connect(uint8_t pin_i2c_sda, | ||
| uint8_t pin_i2c_scl) { | ||
| // Ranges/sensitivity use ICs' default values, then setupSensors() adjusts. | ||
| bool SensorNXP_FXOS8700_FXAS21002::connect(int pin_i2c_sda, | ||
| int pin_i2c_scl) { |
There was a problem hiding this comment.
removed TODO, as connect() calls setupSensors() which now does adjust some sensor parameters.
| if (!cal.begin()) { | ||
| debugE("Failed to initialize calibration helper"); | ||
| while (1) yield(); | ||
| } | ||
| if (!cal.loadCalibration()) { | ||
| if (!cal_.begin()) { | ||
| debugE("Failed to initialize calibration helper"); | ||
| is_calibrated_ = false; | ||
| }else { | ||
| if (!cal_.loadCalibration()) { | ||
| debugI("No calibration loaded/found...will start with defaults"); | ||
| isCalibrated = false; | ||
| } else { | ||
| is_calibrated_ = false; | ||
| } else { |
There was a problem hiding this comment.
Instead of entering infinite loop if calibration setup fails, we set flag saying that data is uncalibrated. Orientation readings will be made, but the debug console will warn that they are uncalibrated.
| Wire.begin(pin_i2c_sda, pin_i2c_scl); | ||
| Wire.setClock(400000); // 400KHz I2C | ||
| cal_.printSavedCalibration(); | ||
| is_calibrated_ = true; | ||
| } | ||
| }//end of loading calibration | ||
|
|
||
| if (!initSensors()) { | ||
| Wire.begin(pin_i2c_sda, pin_i2c_scl, 400000 ); | ||
| if (!startSensors()) { |
There was a problem hiding this comment.
Combined calls to setup I2C and set I2C speed into single call.
| setupSensors(); // TODO - can set ranges, etc | ||
| printSensorDetails(); | ||
| setupSensors(); //set ranges, etc |
There was a problem hiding this comment.
printSensorDetails() removed, as it duplicates what was already done in main.cpp
| accelerometer->printSensorDetails(); | ||
| gyroscope->printSensorDetails(); | ||
| magnetometer->printSensorDetails(); | ||
| accelerometer_->printSensorDetails(); | ||
| gyroscope_->printSensorDetails(); | ||
| magnetometer_->printSensorDetails(); |
There was a problem hiding this comment.
Renamed class variables to closer match Google coding style. Also seen in following lines of code (without a comment).
| //confirm we are connected to sensors, attempt reconnect if not | ||
| if( !startSensors() ) { | ||
| debugW("No connection with orientation sensors!"); | ||
| //assign orientation value that in NMEA2000 means invalid/unavailable | ||
| gx_ = N2K_INVALID_FLOAT; | ||
| gy_ = N2K_INVALID_FLOAT; | ||
| gz_ = N2K_INVALID_FLOAT; | ||
| roll_ = N2K_INVALID_FLOAT; | ||
| pitch_ = N2K_INVALID_FLOAT; | ||
| heading_ = N2K_INVALID_FLOAT; | ||
| accel_event_.acceleration.x = N2K_INVALID_FLOAT; | ||
| accel_event_.acceleration.y = N2K_INVALID_FLOAT; | ||
| accel_event_.acceleration.z = N2K_INVALID_FLOAT; | ||
| return; | ||
| } |
There was a problem hiding this comment.
each time we call for an updated sensor value, we call startSensors(), which confirms that the sensor is still responding to an I2C ping. If not, then it attempts to re-initialize the connection. If no connection is made, the orientation variables are set to special values that NMEA2000 recognizes as "N/A" or "Undefined". This reduces chance that a failed sensor will not be recognized (as opposed to the displays just freezing at the most recent valid value).
| bool SensorNXP_FXOS8700_FXAS21002::initSensors() { | ||
| if (!fxos.begin() || !fxas.begin()) { | ||
| return false; | ||
| //If not already connected to sensors, | ||
| //try to connect and configure them. | ||
| //If already connected, check connection and | ||
| // return true if still responding. | ||
| bool SensorNXP_FXOS8700_FXAS21002::startSensors() { | ||
|
|
||
| if( !is_connected_ ) { | ||
| if( fxos_.begin() && fxas_.begin() ) { | ||
| //the Adafruit begin() routines setup the sensors with these defaults: | ||
| // gyro: 100 Hz data rate, +/-250 dps full-scale, HPF off, active state, no FIFO, | ||
| // default LPF is 32Hz for ODR=100Hz | ||
| is_connected_ = true; | ||
| accelerometer_ = fxos_.getAccelerometerSensor(); | ||
| magnetometer_ = fxos_.getMagnetometerSensor(); | ||
| gyroscope_ = &fxas_; | ||
| setupSensors(); | ||
| } | ||
| }else { //already connected, we believe | ||
| //ping each sensor to ensure it's still working/attached | ||
| if( !pingFXAS21002() || !pingFXAS8700() ) { | ||
| is_connected_ = false; //ping unsuccessful: assume no connection | ||
| } |
There was a problem hiding this comment.
Changed name of initSensors() to startSensors() (to better reflect function). Made the routine more intelligent: if not connected, it calls the sensor begin() methods as before. If already connected (based on results from previous call to fetch orientation data) it pings the sensors to confirm that they are still responding. This way is_connected_ always reflects the most current sensor status, and they can be unplugged/replugged without crashing SensESP. When is_connected is false, the orientation values provided to SensESP are the NMEA2000 values for not-available.
| void SensorNXP_FXOS8700_FXAS21002::setupSensors(void) { | ||
| //TODO: we could set the g range for accelerometer here, for example | ||
| //Sets ranges for sensors, when defaults are not optimal. |
There was a problem hiding this comment.
setupSensors() now sets the gyro data rate to correspond with the sensor's update rate, which means the internal low-pass filter is appropriate for the sampling rate. The default power-on setting is OK for 100 Hz sampling, but can lead to digital aliasing at lower sampling rates.
| //checks I2C bus to see whether FXAS21002 is connected and responding | ||
| bool SensorNXP_FXOS8700_FXAS21002::pingFXAS21002( void ) { |
There was a problem hiding this comment.
Added new functions to ping the sensors (these weren't available in the AdaFruit libraries unfortunately), so we can quickly check whether the sensor is responding before each reading of the data.
| #include "Adafruit_Sensor_Calibration.h" | ||
| #include <Adafruit_FXAS21002C.h> | ||
| #include <Adafruit_FXOS8700.h> | ||
| #include <Adafruit_Sensor_Calibration.h> |
There was a problem hiding this comment.
Moved these two defines into .h from .cpp as they are needed for the Adafruit_FXOS8700 and Adafruit_FXAS21002C objects (which used to be globally variables created in the .cpp file)
| bool connect(uint8_t pin_i2c_sda, uint8_t pin_i2c_scl); | ||
| bool connect(int pin_i2c_sda, int pin_i2c_scl); |
There was a problem hiding this comment.
Fixed a type mismatch with what the I2C calls in connect() are expecting.
| Adafruit_FXOS8700 fxos_; // the combined magnetometer + accelerometer | ||
| Adafruit_FXAS21002C fxas_; // the gyroscope |
There was a problem hiding this comment.
Moved these here from the .cpp file, where they were global variables.
|
OK @ba58smith - I've implemented the changes needed to keep SensESP running during a sensor outage. Testing as described in the first comment of this PR has been done, and it functions OK on a ESP-Wrover board. It also builds OK for the d1_mini and esp32dev targets, but I don't have those boards to test on. In addition to the changes needed for accounting for sensor failures, I also made 2 additional types of changes:
I've added comments to this PR inline with the code changes to hopefully help explain the changes. Let me know of any concerns you have! Cheers. |
|
@BjarneBitscrambler , I got my NXP FX87 sensor in the mail yesterday, I can set it up with a Wemos D1-mini and see that it works. Can do that during weekend. |
|
It all looks good to me, but again, it's so technical, I can't comment on how well it will work. I think it would be good to wait until you test it with the D1 Mini before merging, so after you test, update here, please. Then, given the recent difficulties we've been having with all sorts of newly merged changes - I'll leave it up to @mairas to do the merge. |
|
@BjarneBitscrambler I have now pulled your code and are running it on a Wemos D1-mini on my desk with success. I see that my heading is drifting a lot at a constant rate, have done the mag calibration. But I don't know if that is something that is related to this PR at all. Maybe we can connect on Slack and discuss what I can test? |
|
@tedenda : That's great you have it running on your d1_mini! @mairas can now merge this PR, at his convenience. The drifting heading is something I've seen before, and it's not related to this or any existing PR/Issue. It's happened twice in the last 2 weeks - the rest of the time the heading is quite steady when the sensor is stationary. I haven't tracked down what's causing it yet - my current theory is that there is a zero offset in the gyro data that makes the filter algorithm think there's motion when there isn't. There may also be a scaling issue, whereby the units of the gyro data aren't matching what the filter expects: I believe this because in my testing I also see overshooting, where a sudden motion causes the reported heading to go past the new resting position and then take several seconds to recover. Hopefully with two people now using the sensor we can figure it out quickly :-) I'm not on Slack, but if you want to discuss this over email I think that would be productive and great. You can reach me at bhansen.victoria@gmail.com |
|
@BjarneBitscrambler Email sent, now you have my email too! |
|
I received my FXOS8700 sensor as well last week and build it yesterday. I have it connected to a Lolin D32 and compiling went as a charm, apart from some of my beginner's mistakes. Calibration also very easy, and the sensor works fine. |
|
Good that you got it up and running, we are also one more user that can test an improve code for this sensor. |
|
@DanielG86 Please checkout @BjarneBitscrambler code (this PR) and run that on your board and report back here, if it works fine we can merge this PR and move on with attitude and other things regarding this sensor. |
|
I will take a look tomorrow
Verstuurd vanaf mijn iPad
… Op 24 okt. 2020 om 16:30 heeft Daniel Tedenljung ***@***.***> het volgende geschreven:
@DanielG86 Please checkout @BjarneBitscrambler code (this PR) and run that on your board and report back here, if it works fine we can merge this PR and move on with attitude and other things regarding this sensor.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Just tested it on my board and it all seems fine. Works the same as before. Very stable outputs, except some overshoot on sudden moves, before the data settles on the correct number |
|
@DanielG86 Thanks for testing this PR! |
|
After initial excellent operation, I did some further testing, which showed me some unexpected behavior:
|
|
Update: sawtooth behavior depends on proximity of USB cable. And recalibration does the trick, so forget about item 3 |
|
@DanielG86 I should mount my sensor away from the desk and recalibrate and see if that fixes my sawtooth heading. There is probably some interference that causes it. Take a look on @BjarneBitscrambler guthub, there is a lot to read there about this sensor and maybe you can help with something. |
|
Thanks.
I’ve looked at his wiki pages, and that looks really interesting. I posted an ‘issue’, on his site, offering my support.
Regarding sawtooth, there was a real clear relation with the usb wire I accidentally had really close to the sensor. Quite a big impact of a relatively small current, I would say, and this gives some concern as to when I would take it from the desk to the boat.
Verstuurd vanaf mijn iPad
… Op 27 okt. 2020 om 21:28 heeft Daniel Tedenljung ***@***.***> het volgende geschreven:
@DanielG86 I should mount my sensor away from the desk and recalibrate and see if that fixes my sawtooth heading. There is probably some interference that causes it.
Take a look on @BjarneBitscrambler guthub, there is a lot to read there about this sensor and maybe you can help with something.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
@mairas - do you have time to merge this PR? I'd hate to see it get stale again and overtaken by other changes to SensESP... Just to summarize: this modification has been tested on the ESP32-WROVER, d1_mini, and LOLIN D32 boards. |
|
Sorry, I had failed to follow the PR closely enough! Will merge. I've got an important-ish question, related to Issue #239, though: after this PR, are there any global variables left in the magnetometer code? They are always linked in regardless of their use, and any SensESP program was using 3 KiB more memory after the initial magnetometer code got merged. I saw that you moved two of the globals inside a class, which probably helps a lot, but it's essential that there are none. |
Summary
Issue #151 dealt with a failed BME280 sensor causing the system to fail to start. A similar situation exists with the NXP FXOS8700 / FXAS21002 combination orientation sensor: if I2C communications with the sensor fails, the system hangs. It is preferred that the SensESP program continues to run, to avoid halting data from possible other attached sensors.
Changes Made
Only two files have changed:
sensor_NXP_FXOS8700_FXAS21002.handsensor_NXP_FXOS8700_FXAS21002.cppSensorNXP_FXOS8700_FXAS21002::connect()changed so failure to init calibration helper (which retrieves cal values stored in EEPROM) does not halt program, but rather allows it to continue with warnings displayed on serial console.SensorNXP_FXOS8700_FXAS21002::gatherOrientationDataOnce()always callsinitSensors()before attempting to retrieve data from the sensors, and skips the data retrieval if comms have failed.New Program Flow
Each time data collection is attempted from the orientation sensor, comms are checked. A failure is flagged, which causes future data retrievals to be skipped until the failure condition is corrected. Data retrieval then continues normally until the next comms failure.
Testing Performed
Future Possible Work
Does SignalK define a way to flag invalid data? Currently the orientation data freeze at whatever value they had just prior to the sensor comms failure, and this might not be noticed for a while. I imagine a SignalK alert could be sent out...