[driver] Adding InvenSense 6-Axis IMU driver

[rasmuskleist]

I have created this driver for IIM-42652, but it also works for ICM-42688-P and possibly many other InvenSense IMUs. The only difference between IIM-42652 and ICM-42688-P, that I have noticed from a software perspective is that IIM-42652 has Register Bank 3. Therefore I do not know if Iim42652 is the right name for the device?

In addition I have a couple of points that I would like your take on:

1. I cannot quite figure out what the standard is for naming enum class members, when mapping out registers. I have written e.g. BankSel0 = Bit0 as in Adis16470. However, other implementations e.g lis3dsh the convention is BANK_SEL0 = Bit0. I will happily change the defintion to conform to an enforced standard.
2. Currently I have only added the registers in user bank 0. I presume this is fine?
3. I have only tested the driver with SPI and I am debating if I should only implement the SPI version and remove the transport class?
4. I have not implemented FIFO nor APEX yet. However, I plan to implement FIFO within the nearest future. For me it is fine if this feature will be added in a later PR.

I have some different design ideas for the FIFO interface that I will like your view.

1. Implement a template FifoData class that takes the FIFO buffer size as template argument and inherits from Data. The Data class should then implement functions for retrieving the buffer size and a pointer to the buffer. When constructing the Iim42562 the FifoData class should then be passed instead of the Data class.
2. Implement a template FifoData class that takes the FIFO buffer size as template argument but which does NOT inherit from Data. This class should then be passed to a readFifoData function on Iim42652. However, I think this is might be an awkward solution as the data is not handled in one class.
3. Add a FIFO buffer to the existing Data class, and set the FIFO buffer size using an argument passed to lbuild. Thereby the FIFO buffer and size can be retrieved similar to the sensorData currently is retrieved.
4. Provide the Iim42562 class with an additional template argument to provide the FIFO buffer size. However, I am not in favor of this idea as I find it a bit awkward.

In all circumstances I will add an iterator to the class that handles FIFO data for parsing the data and return parsed FIFO frames. I hope my considerations make sense and thanks in advance!

[salkinium]

Thanks for the driver!

Therefore I do not know if Iim42652 is the right name for the device?

You can add the compatible devices into the module description then it is also findable via the homepage.

1. There is no real standard. For a while we thought that CamelCase would be great, but when converting ABBREV register names, it sometimes becomes unclear which letters to capitalize and which not. I would recommend to use the convention in the datasheet, at least this is canonical.
2. Yeah, sure. You could add a admonition to the lbuild docs to make it clear that this intentional. That makes it clearer for other developers.
3. No, leave it in and put a warning in the lbuild docs via admonition that it's untested.
4. I'd probably prefer option 1, however, you can also allocate the Fifo buffer externally and pass it to the Data class, which can deduct the size and store it as a std::span. So effectively a runtime mechanism.

I'll review in detail before the end of the week this still gets into the 23Q2 release.

[rasmuskleist]

Thanks for the nice comments! I have addressed point 1, 2 and 3 in the latest commits. I plan to address point 4 tomorrow with my first suggested design. Also I was reading the documentation for ICM-42688-P in greater detail and found that some registers are not identically the same as for IIM-42652. For example the APEX_CONFIG0 is a little different between the two devices. Therefore I have not added an admonition that the driver is compatible with e.g. ICM-42688-P. Is this still relevant or should I leave it as is?

[rasmuskleist]

I have started the FIFO implementation and have had some problems with the endianess of the imu which I will have to look into. Specifically, I configure the device to use little endian for both sensor data and fifo count. However, setting the fifo count to use little endian, seem to have the oppostive effect. Every time that I have configured the imu to use little endian and have used (FIFO_COUNTH << 8) | FIFO_COUNTL get a byte count in excess of 4000. That is despite the FIFO only having 2K bytes. I will have to look into this tomorrow.

[rasmuskleist]

I have had some trouble getting the FIFO to work. The trouble was caused by setting the FIFO_WM_GT_TH bit in FIFO_CONFIG1 or setting TMST_EN in TMST_CONFIG. Altough I would not have expected setting these bits high would have caused the FIFO_DATA register to contain zero readings. However, currently I am getting readouts through FIFO so now I only need to parse the data, which I have already started.

[rasmuskleist]

I have been a bit busy the last week and hence the slow progress. I have now implmented a parser for parsing the fifo data and an iterator which uses the parser to iterate through the fifo data. I will now have to test that the iterator works as intended. Also I will have to figure out if there is a nice way to pass the scale onto the FifoPacket class or if this should be up to the user to provide the scale.

[salkinium]

Very thorough driver, impressive! Just some minor nitpicks from me.

[rasmuskleist]

Thanks for the nice comments! It has taken a lot of energy getting the driver to this state. I choose to rename the driver as in this repo. I believe this is fine? I might ad an admonition that the driver is based on iim42562 and that some registers might be missing or not applicable to other InvenSense devices?

What is missing now is to somehow pass the accel and gyro scale onto the FifoPacket or in another way implement code that can convert the raw readings to some scale with physical meaning. The obvious choice is to have FifoData pass it onto the iterator which passes it onto the FifoPacket. However, I think that is a bit clumpsy. Do you have any suggestions?

In addition, I have yet to implement support for APEX data. However, this outside of my personal use case, so I prefer to add it at later.

Finally, I was debating if I should remove the ixm42xxx.transport module and let the ixm42xxx module copy the transport files? I believe the intention behind lis3.transport was to make it available to multiple drivers for example lis3dsh and lis302dl, which have different implementations.

[salkinium]

The renaming is fine, especially with documentation about possible limitations and tested versions.

You can remove the separate transport module, we can add it back if needed, just leave the classes in separate files.

I'm not sure about the how to pass the scale, I don't think you have much choice unless you convert it before passing it to the packet.

[rasmuskleist]

I now consider this ready for merge unless you have any additional changes? Before merging I suppose that I will change up the git history to have:

[math] Adding long vector typedefs
[driver] Adding InvenSense 6-Axis IMU driver
[example] Adding InvenSense 6-Axis IMU example
[driver] Adding InvenSense FIFO support
[example] Adding InvenSense FIFO example

I have choosen to implement the FifoPacket scaling methods by passing the scale to the method. This servers two purposes. First of all I avoid having to pass the scales twice. More importantly the scales are stored only in one place, so there are not multiple sources of truth. What do you prefer?

Also I should add that the when compiling I get -Wdouble-promotion warning from the printf statements. I suppose that I should simply ignore this?

[chris-durand]

Also I should add that the when compiling I get -Wdouble-promotion warning from the printf statements. I suppose that I should simply ignore this?

If you pass parameters to a C-style variadic function like printf they undergo "default argument promotions" and float is converted to double. We might have to turn off that warning. There would be ways to explicitly suppress the warning but it would be way too verbose and noisy to put that around every printf call. Just ignore it for now.

[salkinium]

Excellent driver! Please rebase/squash as you described.

[chris-durand]

Thank you!

[chris-durand]

Thanks!