Change Orientation Sensor to use v7 Library

[BjarneBitscrambler]

Summary

The AdaFruit AHRS (Attitude and Heading Reference System) library used for the Orientation sensor employed the NXP v4 sensor fusion algorithm. An updated NXP v7 library is available, and the SensESP Orientation sensor code has been rewritten to take advantage of it. There are several improvements that this change introduces:

• the new library has run-time magnetic calibration so an external calibration tool and steps to use it are no longer needed
• there is a reduced dependence on external libraries (removed 4, added 1)
• the SensESP portion of the orientation code is simpler (replaced 4 files with 2)
• orientation is now available on the Signal K navigation.attitude path, providing yaw, pitch, and roll as prescribed in the Signal K spec. Compass heading is now available on the navigation.headingCompass path, also as prescribed.
• now have an example of how to add new SKOutput producers that emit more than one parameter in the Signal K path
• the SensESP orientation code now adheres to this project's coding style (Google C++ Style Guide) (barring any hopefully minor deviations I've missed)

Issues Addressed

When this PR is completed, it addresses these issues:

• closes SensorNXP_FXOS8700_FXAS21002 code must be formatted according to project standards #244
• closes Update fx8700_heading_example to write to correct SK Path #216
• closes Update FX8700 example #225
  The following issues were kept in mind while making the changes (but shouldn't be closed as they apply to other sensors as well):
• Add metadata to examples that output Signal K numeric values #322
• Review all DynamicJsonDocument sizes, adjust as appropriate #320

Changes Made

• The example main.cpp (found in examples/fx8700_heading_example.cpp) is updated
• library.json and platformio.ini have three unneeded libraries removed and one added
• the four sensor files (orientation_9dof_input.* and sensor_nxp_fxos8700_fxas21002.* )have been consolidated into two: orientation_sensor.h and orientation_sensor.cpp
• new file signalk/signalk_attitude.h contains the struct definition for Attitude data, and a typedef ValueProducer<Attitude> AttitudeProducer. These could be moved into signalk/signalk_output.h to eliminate this new file.
• signalk/signalk_output.h now contains the template specialization for SKOutput<Attitude>:: and a typedef SKOutput<Attitude> SKOutputAttitude

Testing Performed

• The code compiles and runs on the ESP32-WROVER and ESP8266 d1_mini platforms.
• A Signal K server running on my Raspberry Pi receives the expected data. The Instrument Panel widget interprets the Attitude data (navigation.attitude path) correctly as yaw, pitch, and roll and automatically converts the units to degrees. The widget also shows the compass heading (navigation.headingCompass path) correctly and likewise converts the units to degrees.
• Handling failure and recovery of sensor communications seems to work: interrupting I2C comms to the sensor does not cause the application to hang, and Signal K messages during the interruption contain the special JSON value null indicating invalid data for the Attitude fields. The Signal K Instrument Panel displays -.---- for yaw, pitch, and roll during the interruption, and reverts to proper values when comms are restored. The compass heading has not yet been modified for this behaviour yet.

I and a couple other users are currently testing in more depth.

Status

It would benefit from outside review and testing, so please dig in. Thanks!

[mairas]

I haven't had reviewed any other bits of the PR yet.

[mairas]

This line will make the whole SensESP project hostage to any changes in your private library. If you do any change in that library that will break SensESP for everyone. So, obviously not acceptable.

Instead, you could split the whole orientation sensor into a separate library. This would reduce the number of SensESP dependencies, make SensESP builds go faster in the common case and decouple orientation sensor development from the common SensESP project. Also, this is something that is likely to happen for all sensor libraries in SensESP 2.0.

[BjarneBitscrambler]

I don't understand. There is only one point of interface between the Orientation library and the SensESP project, and that is the #include "sensor_fusion_class.h" in the file orientation_sensor.h And orientation_sensor.h is only used by the orientation example (examples/fx8700_heading_example.cpp). So if the Orientation library were to disappear, then compilation would continue after deleting the orientation example, and all other examples would function exactly as before. That's hardly all of SensESP being held hostage. In fact, the proposed changes require three fewer external libraries than before, which reduces the number of possible failure points.

Can you explain what you mean by "split the whole orientation sensor into a separate library", and how that differs from what is proposed here?

One key point that initially attracted me to the SensESP project was the availability and quality of ready-to-use sensors. I feel changes that make it harder for someone new to Signal K and SensESP to get going quickly are going to reduce the number of users.

[ba58smith]

@BjarneBitscrambler , we have started the process of breaking out SensESP sensor code into separate libraries, each with its own GitHub repo. As soon as we decide where to put these, I'm going to start working on the first one, which hopefully won't take too long. The process should be pretty simple, as will the implementation of this structure - no different from any other library we've used in SensESP, except that there will be no reference to a particular sensor anywhere in SensESP code - only in the main.cpp and platformio.ini of a particular project. I will hopefully have some guidance for you soon.

[BjarneBitscrambler]

Thanks Brian for the explanation. I look forward to hearing more.

I also want to mention that, while the majority of this PR's changes are captured in the orientation-specific code files, there were a few changes to SensESP files as well. These were changes that appeared necessary to support the requirements of the Signal K navigation.attitude output. I implemented them as seemed best to me, but will appreciate hearing whether there are better options. I'll include amplifying in-line comments in this PR hopefully later today; FYI the specific files are:

• src/signalk/signalk/signalk_output.h
• src/signalk/signalk/signalk_attitude.h (curious whether this would be better included in signalk_output.h, or elsewhere)

[ba58smith]

@BjarneBitscrambler, I looked at the code for navigation.attitude, but it's beyond my experience to comment on it. I'll have to leave that up to @mairas or @joelkoz.

[mairas]

To continue on my original comment and the related counter-questions: it doesn't even really matter whether a dependent library is used or not; it will break the build if there are any syntax errors or, say, namespace clashes with other libraries. If any such issues are accidentally introduced in your library, that will break SensESP without warning for everyone. That is why I don't think such an approach is workable.

[ba58smith]

We've been putting all of the nice Doxygen-style comment only in the .h file. If you have them all there, as well as here in the .cpp file, you can delete them here. The only comments we've been leaving in the .cpp files are comments that might be necessary to explain how a function works, or why it's necessary, etc.

[BjarneBitscrambler]

I hadn't been duplicating the comments, but rather documenting the class as a whole in the .h file, and the individual methods in the .cpp file. I can move them all into the .h file if you prefer - should I?

[ba58smith]

If any of these don't initialize / install, does it make sense to output all of the values? Some of the values? I know we haven't done this kind of checking before, but @mairas just asked me about it for a PR I did for the MAX31856 thermocouple sensor. Have a look at it, and see if you think it makes sense to do something like that for this code.

577bcf8

[BjarneBitscrambler]

For sensors that have multiple parameters but not all of them are valid/functioning, then my opinion is to provide the ones that are available and flag the ones that aren't. For flagging, the Signal K spec (v 1.5.0 pg 14) says that data providers are to send a JSON null in their delta message to flag invalid values, but is not clear on what to do if only some of a group of parameters are invalid.

So to make this happen, I'm going to have the Orientation library use the value that NMEA defines for invalid/unavailable float values (#define N2K_INVALID_FLOAT (-1e-9)) and put that into any of the orientation parameters that aren't available. Then the SensESP code that formulates the Signal K message can substitute in a NULL in the JSON string.I haven't implemented that yet, but that's the plan...

For some computed orientation values (e.g. heading) valid values are always positive, so there should never be a NULL appearing in the JSON output unless the parameter really is unavailable. Some other parameters (e.g. acceleration) can genuinely go negative, so there is a small but finite chance of sending out a valid -1e-9 value that is then overwritten by NULL in the translation to Signal K. My feeling however is that such a value is going to be very transient (orientation is always bouncing around, even on a stationary surface and more so on a boat). So the display of that parameter might be temporarily flagged as unavailable, but would quickly be replaced by the next valid value a fraction of a second later. At least that's the theory - have to see what happens in reality. Another more conservative approach could be to intercept legitimate -1e-9 values in the orientation library and replace them by something almost indistinguishable to a human (e.g. -2e-9). This would reserve the -1e-9 value for those that are missing/invalid.

What do you think? Thanks for bringing it up...

[BjarneBitscrambler]

I've modified the behaviour of SKOutput<Attitude> so it now deals with invalid/unavailable values in the manner Signal K prescribes. When the orientation sensor is unavailable, the delta messages contain JSON null instead of numeric values in the appropriate fields. I've confirmed the Signal K server's Instrument Panel behaves as expected, displaying -.---- instead of a number for the duration of the unavailable data.

To implement this, I decided not to use a "magic number" as the means for the data Producer (sensor) to tell the Consumer (SKOutput) that a value is invalid. It seems too fraught with special cases needed to deal with the variety of data ranges and types that can be passed. Instead of a magic number, I went with an out-of-band signal. In the case of the Attitude data, which was passed as a struct anyway, I added a struct member is_data_valid.

I'm unsure how best to adapt this approach to deal with single-value Producers (i.e. where the output variable is an int, float, boolean, etc). We can go with the magic number approach, or change the output variable to a struct for even the single-value outputs, or.... Perhaps someone can think of a clever modification of the Producer class to accomplish this.

[ba58smith]

@BjarneBitscrambler, when you're ready, you can move this all into a new repo in the new SensESP Organization (https://github.com/SensESP), like https://github.com/SensESP/INA2xx. In that repo, you should find everything you need to make this move - most of it in the Wiki: https://github.com/SensESP/INA2xx/wiki/DRAFT:-How-to-Create-a-SensESP-%22External-Library%22-Like-This-One .
Let me know what you'd like to call the repo (keep is short and simple, please), and I'll create it, and give you the necessary rights to create and merge PR's to it.

[DanielG86]

Great improvement with the calibration saved through the web interface! With this, the sensor is now really usable in a live setting. Thanks, Bjarne!

[BjarneBitscrambler]

Fantastic @ba58smith, thanks! I had a quick look at how you set up your example and it seems pretty clear.

For the repo name, how about orientation? I was pondering whether to use the IC part number or not, but decided for two reasons that that isn't the best idea: first, it's a combo of 2 ICs and the full part number is FXOS8700/FXAS21002, which is rather unwieldy; and second, the ICs used for motion sensing are evolving pretty quickly at this point in their life-cycle (unlike, say, temperature sensors), and I suspect the manufacturers will come out with new ones. The orientation code itself is fairly sensor-agnostic and I don't object to including new ICs, but changing the repo name each time strikes me as an annoyance. What do you think?

[ba58smith]

@BjarneBitscrambler - I'm happy with orientation. It's been created, and you should have received an email inviting you to be a Collaborator. https://github.com/SensESP/Orientation

[mairas]

@BjarneBitscrambler - I'm happy with orientation. It's been created, and you should have received an email inviting you to be a Collaborator. https://github.com/SensESP/Orientation

In my opinion, the repository needs to be renamed to something more specific. The code at the moment is tightly integrated to some particular hardware and I don't want the name to imply that it's more generic than it is.

[ba58smith]

@mairas, I agree that a more specific name would be nice, but what? orientationFXOS8700/FXAS21002?

[ba58smith]

@BjarneBitscrambler - we're having discussions about exactly how to set up these repos, so I've canceled your invitation to be able to edit it. When we decide on the setup, including authority levels, I will issue a new invitation. Sorry for the confusion.

[BjarneBitscrambler]

In my opinion, the repository needs to be renamed to something more specific. The code at the moment is tightly integrated to some particular hardware and I don't want the name to imply that it's more generic than it is.

@mairas - I strongly disagree with your statement. The old Adafruit library was tightly bound to the hardware, however when I ported the NXP fusion library over from their Kinetis processors to the ESPs, I paid a lot of attention to segregating accelerometer/magnetometer/gyro IC functionality from the fusion algortihm. The Orientation library is easy to adapt to a new sensor IC - lots of details in the online documentation over at https://github.com/BjarneBitscrambler/OrientationSensorFusion-ESP

How many people, when they search for a sensor solution, say to themselves "I have an XYZ123 chip...what can I use it for?" It's much more likely that someone will search by asking, "What sensors will give me current measurement? What sensors will give me orientation? etc..."

[mairas]

@mairas - I strongly disagree with your statement. The old Adafruit library was tightly bound to the hardware, however when I ported the NXP fusion library over from their Kinetis processors to the ESPs, I paid a lot of attention to segregating accelerometer/magnetometer/gyro IC functionality from the fusion algortihm. The Orientation library is easy to adapt to a new sensor IC - lots of details in the online documentation over at https://github.com/BjarneBitscrambler/OrientationSensorFusion-ESP

As long as the interface to the hardware sensors is not via some generic hardware abstraction layer but via I2C, I don't see how you could get it working with, say, a bog standard MPU9250 module. I2C is not an abstraction layer.

How many people, when they search for a sensor solution, say to themselves "I have an XYZ123 chip...what can I use it for?" It's much more likely that someone will search by asking, "What sensors will give me current measurement? What sensors will give me orientation? etc..."

Yeah, and if they take something as MPU9250 and then try to see what libraries are there available for SensESP, they're for sure going to be confused if they found something that claims to be a generic orientation library and is anything but.

Don't take me wrong: a proper sensor fusion library is great to have, and if I had time, I'd love to play with them myself. I just don't want libraries landing in the SensESP organization (under github.com/SensESP, that is) to have too generic names because that's misleading.

[BjarneBitscrambler]

As long as the interface to the hardware sensors is not via some generic hardware abstraction layer but via I2C, I don't see how you could get it working with, say, a bog standard MPU9250 module. I2C is not an abstraction layer.

From the specs of MPU9250, it actually seems quite compatible with the orientation library and I'd be willing to add that part if someone has a genuine interest in it. Or they can do it themselves; all that's required is writing functions to initialize the part and read from the part. Though it's currently done with I2C (which the MPU9250 supports), it doesn't have to be. SPI, which many sensors also support, would be easy.

How many people, when they search for a sensor solution, say to themselves "I have an XYZ123 chip...what can I use it for?" It's much more likely that someone will search by asking, "What sensors will give me current measurement? What sensors will give me orientation? etc..."

Yeah, and if they take something as MPU9250 and then try to see what libraries are there available for SensESP, they're for sure going to be confused if they found something that claims to be a generic orientation library and is anything but.

Several comments:

• I'd ask that person whether they are trying to solve a problem (i.e. getting orientation), or trying to find a use for a part they happen to have. Product development should focus first on defining what the goal is, and only then searching for how best to achieve it. Doing it in the opposite order is more akin to hacking about.
• If there's confusion over what hardware is needed, then reading the documentation will alleviate it. It's much more informative having a functional description of a library, than a library named after an IC part number.
• Renaming libraries is a pain, as is the alternative of cloning a new one each time the hardware changes. You cite the example of the MPU9250: that part is obsolete and has been replaced by the ICM-20948 (see https://invensense.tdk.com/download-pdf/migrating-from-mpu-9250-to-icm-20948/). So had a library been named MPU9250, it would already need to be changed. Or else a new library created alongside the previous one (with the attendant headache of maintaining multiple almost identical libraries). It's easier just to update the documentation and add the new part(s) to the list of compatible hardware.
• Many ICs have ambiguous names containing various combinations of spaces and hyphens, as well as pre- and post-fixes indicating packages. Referring again to your example of the MPU9250: it's actual name is MPU-9250. A strict search for an MPU-9250 library wouldn't come up with the MPU9250 library, based on the library name. Better to have IC names, and their variants, called out in the documentation and tags for the library. It gets even worse for multiple-IC sensors, like the FXOS8700/FXAS21002: do we include a space, an underscore, a forward slash, or a hyphen between them?

Don't take me wrong: a proper sensor fusion library is great to have, and if I had time, I'd love to play with them myself. I just don't want libraries landing in the SensESP organization (under github.com/SensESP, that is) to have too generic names because that's misleading.

Your call, but I really think including the IC part number(s) is a poor choice.

[mairas]

Once more: I2C is not a hardware abstraction interface. SPI is not a hardware abstraction interface. Serial bus is not a hardware abstraction interface. Onewire is not a hardware abstraction interface. Ethernet is not a hardware abstraction interface. They are communication protocols and they by themselves in no way hide away the specific details on how to exchange information with some particular class of devices in a device or manufacturer agnostic manner. If you don't abstract away your hardware devices from your implementation, it is not generic by any means.

At this point I believe the least painful option for all of us is that this code is hosted under github.com/BjarneBitscrambler instead of the SensESP organization. There really is no downside to it for you -- functionally it makes no difference where the repo is, and you get to do all code design decisions according to your own liking. In contrast, if the repo were hosted under SensESP, all code changes would still need to go through PRs, which obviously isn't working for any of us.

[BjarneBitscrambler]

Let's do that.

[BjarneBitscrambler]

If you arrived here looking for the orientation code using SensESP - you'll find the latest version at https://github.com/BjarneBitscrambler/SignalK-Orientation Cheers, Bjarne.