Support for factory-programmed firmware

[mboulet]

A driver for the Sparkfun Razor IMU M0 with the factory-programmed firmware can be found at https://github.com/mitll-ros-pkg/razor_imu_m0_driver .

This driver differs from the razor_imu_9dof package in that it only outputs the sensor measurements, follows REP-145, and interfaces with the factory-programmed firmware.

Is there interest in a pull request to merge these packages? I.e. the razor_imu_9dof package provides two nodes: the node as it currently exists and a second "factory firmware" node.

Given that the packages provide different functionality, i.e. an AHRS vs simple driver, it may make sense to keep the packages separate. On the other hand, merging the packages could reduce user confusion.

[lebarsfa]

There are some DEBUG_XXX options to enable to test e.g. DMP functionalities. For me the default factory firmware seemed to have some bugs if used as is. However following REP-145 should be probably a good idea...

[mboulet]

@lebarsfa Sorry, I don't follow what you mean by mentioning the DEBUG_XXX options. I did find that the attitude (quaternion) output of the device was erroneous when the angular rate full-scale range was not 2000 deg/sec. Although I'm not sure whether that was a firmware bug or inherent to the IMU hardware. If you recall any details of the bugs you found, I would be happy to try and reproduce them.

[lebarsfa]

In SparkFun_MPU-9250-DMP_Arduino_Library, there were some doubts about the computations in computeEulerAngles(), especially the pitch (*2?) and maybe the use of an unusual coordinate system or definition for the roll, pitch, yaw, see sparkfun/SparkFun_MPU-9250-DMP_Arduino_Library#5.
In computeCompassHeading(), there is a 180 instead of a PI/2.0, as discussed in sparkfun/SparkFun_MPU-9250-DMP_Arduino_Library#6, note however that the conclusion of the discussion with a pull request might be also partially wrong, when you get atan2(y,0) it should be +/- PI/2.0 instead of 0 or PI, in the code we see atan2(mx, my) probably because they want to convert the coordinate system, my suggestion is just to not make any check since the atan2() function should theoretically handle correctly the particular cases (and it seems to do it in practice when I tested)...
Additionally, since the device has a micro USB and micro SD ports that are in a material naturally magnetized, the value returned by the function is never correct in practice, we really need a magnetic calibration algorithm if we want to rely on magnetometers.
Potentially uninitialized variable : sparkfun/SparkFun_MPU-9250-DMP_Arduino_Library#2 (maybe in practice some compilers would always default to an initilization of 0, but maybe some not, I don't know...).

In _9DoF_Razor_M0_Firmware, ENABLE_SD_LOGGING is defined twice for different purposes.
Suggestion to add dmpFeatureMask |= DMP_FEATURE_GYRO_CAL in initIMU() in sparkfun/9DOF_Razor_IMU#10.
Some flashenableTime that should be flashEnableTime.

Before working on the AHRS firmware here, I forked a version of both SparkFun_MPU-9250-DMP_Arduino_Library and _9DoF_Razor_M0_Firmware with corrections of the problems I have mentionned (see https://github.com/lebarsfa/SparkFun_MPU-9250-DMP_Arduino_Library and https://github.com/lebarsfa/9DOF_Razor_IMU), so it should work also with your ROS node (maybe after tweaking config.h to choose the desired default input and output modes). And in the AHRS firmware here, there is a section with debug options in https://github.com/KristofRobot/razor_imu_9dof/blob/indigo-devel/src/Razor_AHRS/Razor_AHRS.ino, that enable also to use the DMP like it is in the default firmware, so in the end the 2 firmware can provide the same kind of data using the same internal algorithms if needed.

However, although I am not a specialist in ROS, I looks good to me to make the AHRS firmware follow REP-145 (like one of the Xsens MTi ROS node, the CHR-UM6 and I hope soon the SBG Ellipse), does anyone else feel the same?
Maybe indeed the best should be that the ROS code works transparently whatever firmware is in the device for basic IMU functionality, so I think I would be in favor of a merge like that...