Closed rasmuskleist closed 1 year ago
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.
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.
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?
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.
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.
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.
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.
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.
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?
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.
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:
I have some different design ideas for the FIFO interface that I will like your view.
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!