sensors: Implement a vertical decoder API

[yperess]

Introduce an alternative decoding scheme that focuses on vertical vs horizontal decoding. This PR is based ontop of #60063 so only the latest commit should be reviewed.

PR was merged, this is ready for review.

[yperess]

@teburd please let me know if you want to sync on this (this is just a draft, you can test this out on the akm09918c on the TDK board).

I've been talking to several teams internally at Google and I believe that a vertical decoding scheme would benefit a larger majority of applications when compared to our existing horizontal decoding scheme. Please reference #60944 for visualization.

[yperess]

This now works with the icm42688 one-shot reading. Streaming support will come shortly.

[yperess]

Based ontop of #61186, so I'll need to rebase this once that PR lands. But this is pretty much ready for review.

[yperess]

@teburd Tristan was backed up with breaks from the ISH hal changes and did finish the tests for this PR, they'll come next week. Any chance you can give this change a quick review with the assumption that the tests will pass early next week? This will unblock rebasing the streaming API so we can focus on that review.

[tristan-google]

I will refrain from posting a review since I wrote a decent chunk of this code but am available to answer any questions/concerns.

[MaureenHelm]

There seems to be a lot of channel-specific additions like this throughout the PR. Have you measured the impact to code size?

[yperess]

I have not, specifically because the roadmap is to eventually deprecate the old APIs and then all of these will go away. Happy to measure of you'd like.

[yperess]

OK, I ran: $ west build -p -b tdk_robokit1 samples/sensor/sensor_shell/. Summary:

• Flash increase: 1,140B
• Ram increase: 0B

I think that with the approach we chose of having this augment the existing API, there's no way to avoid the bloat until we deprecate the old API and do the final steps of migration. At that point the tradeoff should be

• adding RTIO constructs
• adding shared memory pools

• removing buffers from the sensor drivers
• removing dedicated threads for interrupts

I believe the net increase in size should be very minimal, even depending on configurations (since the shared memory pools are shared across all sensors that the application wants) the memory footprint might be smaller than we started.

Without this change:

Memory region         Used Size  Region Size  %age Used
           FLASH:       87560 B         2 MB      4.18%
             RAM:       19144 B       384 KB      4.87%
        IDT_LIST:          0 GB         2 KB      0.00%

With this change:

Memory region         Used Size  Region Size  %age Used
           FLASH:       88700 B         2 MB      4.23%
             RAM:       19144 B       384 KB      4.87%
        IDT_LIST:          0 GB         2 KB      0.00%

[teburd]

Can you show the rom report? If the rom increase is almost entirely in the sensor shell that seems, at least from my perspective, less of an issue and one that can be worked on.

The 1kb rom increase is solely from this vertical decoder change is that right? Can you annotate this with the git sha's?

[yperess]

rom_report.base.txt
rom_report.diff.txt
rom_report.new.txt

[yperess]

The diff is almost entirely in the sensors (since they need to support both code paths) and in the shell

[MaureenHelm]

I expect some overhead in supporting old and new APIs simultaneously. My question was about the code impact in changing from horizontal to vertical decoding in the new API.

[yperess]

@MaureenHelm @teburd OK... I did some more exploring and this increase is 100% a part of supporting 2 data representations. I took the icm42688_decoder.c as the example because we had a good before/after fully implemented decoder. The cost there comes from decoding to the old sensor value then to the q31. When (locally) I changed it directly to the q31 the decoder was actually 264B smaller (27.5%). It's not entirely a fair comparison because the old decoder would also benefit from exactly the same change (converting directly to q31 format).

I'm happy to make the change, but while we support 2 code paths on these sensors I think it's cleaner to use the common representation for readability. Later when we deprecate the old fetch/get we can optimize back down.

[teburd]

I'm ok with this, I think mostly the code size should be about the same. Theoretically the only significant change would be the way the frame iterator becomes an index into the buffer to copy out. That really shouldn't be a big jump in code size if at all. At an application level maybe slight more memory (the size of the decoder context) is used per channel. This again shouldn't make or break a memory budget in my humble opinion, and can be placed on the stack.

Let’s not accidentally merge while discussing

[fabiobaltieri]

@MaureenHelm can you take a look? This is a big change in sensor.h

[yperess]

@MaureenHelm can you take a look? This is a big change in sensor.h

Maureen and I discussed this offline, she requested that I tweak the sensor_shell.c changes a bit so that the print handling isn't per channel but instead per data type