IMU traits

[Noxime]

Modifies IMU trait to actually return the IMU data and adds FusedIMU trait to access quaternions
Removes feature level IMU selection

[TheButlah]

There's several problems with this PR unfortunately. I'm going to list them out just to give the feedback, and then close the PR. The reason why I'm closing the PR is so that I can open a new PR implementing the fixes, and its a bit easier (and safer against unbeknownst force pushes) to do that in a separate PR.

Concerns:

1. I'm not keen on having an option for mag data, that means that we have to check that at runtime in hot loops. maybe branch prediction could save us but I don't like the idea. I'd rather have the existence of the mag encoded at the trait level. Since I also don't want to deal with the mag at all yet, I'm going to just remove that. I don't expect we will support the mag unless someone really wants to do it, but I want the non-mag approach fully proved out first.
2. Imu should be RawImu, as some imus output fused data.
3. nb2a function was useful, I'd like to keep it. We are still likely going to need it for stuff, unless I'm just missing something.
4. At first when I heard you removed the imu features, I thought that you meant that you implemented some new system to make them unecessary. But you actually just fully removed the ability to use other imus, lol. Now the only imu we can test is the bmi160. I'm not sure why you made this change, especially in a PR that I assumed would be just about the imu traits

Comments:

1. I was not sure about having using async fn in trait, especially as the esp32 builds are still broken and those are 6 weeks behind the latest nightly (they only build the esp toolchain on each stable compiler release). But unfortunately I just don't see an efficient way we could do async i2c without using async. So I suppose its a necessary evil here, we will be opting into a bleeding edge feature that may not really be supported on esp32. Hopefully I'm proven wrong tomororrow when the latest rust stable release comes out, with esp32 updating to async fn in trait shortly after.
2. There are multiple features in this PR - looks like its not just the IMU traits but also new bit patterns for acceleration fsr in bmi160 and reading the gyro from bmi160. Ideally those are their own PR. I know you wanted to implement the trait as part of the refactor, but I think it could have been done with two PRs.

Pog stuff:

• I like the Naive fusion type. Its simple and a lot better than me pretending that euler angle velocities were quaternions, lol. I do think its going to need to be changed soon since using a quaternion as a velocity will overflow/underflow if the velocity is too high. But this improves on what was there before, so I'm fine with it till we have something better.

[TheButlah]

I made a few refactors in the code. I didn't want to proceed with my own implementation because you already worked hard on this and to submit my own code would be both redundant and also snub you of authorship. So I was wondering if you would be willing to rebase this PR on main and make the changes above.

Alternatively, I could proceed with my own implementation if that is more convenient for you.

The changes I made to main that are relevant here and a rebase should avoid clobbering:

• No longer feature gating the imu mods, driver dependencies are unconditional. But we still have the features for imus so we can control which we select. Can change that to env vars later, that is out of scope for this PR.
• moved the drivers to driver mod
• Renamed Imu to FusedImu

[Noxime]

Re: Concerns

1. I do want to support feature parity with legacy firmware at minimum, and hopefully beyond. Adding what I know we'll need in the future (consumed in Add BNO055 #157 ) helps us see what the requirements are and merging it to main now "lets others build on it". I would not make any assumptions about microcontrollers including any branch predictors, but that being said I doubt this will be a performance issue. If it is, I just propose we upgrade our minimum spec to the level of the ENIAC, which should be able to handle the overhead for about 50 IMUs (5000 boolean comparisons/s)

2. For IMUs that produce fused data, there is the FusedImu trait. Calling this trait RawImu would imho be misleading as it does not output raw imu data. The data has been normalised to common metric units and possibly even ran through pre-fusion filters, like lowpass

3. I don't see why we should write code in an append-only manner. Luckily our version control system keeps track of file history and the function can be copy pasted back if the need arises

4. Changing one line in an .rs ending file is equally easy as changing one line in a .toml ending file, and former lets us run workflows on all IMUs at once instead of a matrix. This is good because it exposes issues like writing self::imu::ඞ::math, (only the bmi160 integration implements driver crate functionality in firmware). This change would not have been necessary if the pre-required architectural functionalities were implemented first, but you suggested IMUs should be worked on first.

Re: Comments

1. We already require Rust nightly. If adding new nightly features is to be avoided, this can be replaced with async_trait crate until we move entirely off alloc

2. Adding an API with no consumers or implementors is pretty useless, for added commit history noise.

Speaking of that last point, I do hope you clarify the contribution guidelines. As you can tell, I have a lot of beef with how it is dealt with currently, and is actively making me not want to go through the hoops of PRing.

My current understanding of our collaboration policy:

1. PR even if the feature is incomplete
2. Do not PR incomplete features
3. You can PR code that is known to be broken to main (Pog section)
4. You can't PR code that might be broken to main
5. You can break main
6. You can't break main (comments/2)
7. We use continous integration workflow to avoid conflict pain
8. However, because of this approach I now have to rebase 6 PRs within 12 hours of opening them
9. Your time is wasted because of the contribution policy
10. You do not participate in the contribution policy
11. Don't force push
12. Do force push

I really do hope you have the time to work out the contribution guidelines. Because as it is, there is no way for me to work that is efficient, produces good code and is enjoyable. Jumping through these hoops takes a lot of energy and I dont have the same monetary motivators like you do. And when im very frustrated, I become very annoying, which surely saps your mood too

[TheButlah]

For IMUs that produce fused data, there is the FusedImu trait

Sorry let me clarify what I meant. I realize what I communicated was misleading. I meant that we should have two traits, a RawImu and a FusedImu. FusedImu currently exists on main.

I do want to support feature parity with legacy firmware at minimum, and hopefully beyond. Adding what I know we'll need in the future helps us see what the requirements are and merging it to main now "lets others build on it".

Yes I agree. But this PR removes all feature flags for imus, I'm not advocating not merging in imu support, I'm advocating not mixing those architectural changes in with other unrelated features like imu traits and fusion algos. You can merge sensor fusion and imu support just fine under the existing system without eliminating the imu feature flags.

But maybe your concern was that you don't want to have to add more CI jobs, as you say here:

Changing one line in an .rs ending file is equally easy as changing one line in a .toml ending file, and former lets us run workflows on all IMUs at once instead of a matrix.

I agree that the many ci jobs is silly, especially as its just building the code. In #161 I introduced a change that removes the feature gating of the modules and dependencies, while retaining the feature flags for easy testing. I desire to switch from feature flags to env vars soon, but I do not wish to need to directly edit the .rs file to choose the imu. #161 is my preferred stepping stone towards env vars, for now. Because the branching of the code is so minimal there, we can easily switch CI to only test imu-stubbed in a followup PR. I'll introduce that change shortly and we can rebase the other imu PRs on that so they don't need to worry about configuring feature flags in CI.

We already require Rust nightly. If adding new nightly features is to be avoided, this can be replaced with async_trait crate until we move entirely off alloc

I'm not willing to adopt the use of the async_trait crate in embedded firmware. Separately, its been a policy that I communicated in discord that I want to adopt rust features only after the esp32 toolchain gets them. Maybe I didn't make that clear though, or maybe the message got lost in the channel. I think I discussed this more with @ImUrX in dms so thats probably why you didnt realize that is what I was going for. I'll also try to make that clear in contribution guidelines but for now know that I want esp32 builds green in CI, and we are day(s) away from that being possible now.

All of that being said, I said that I'm fine with the use of async fn in trait. Thats why it was in the "comments" and not "concerns" section. I was explaining my rationale on why I was accepting the change.

I don't see why we should write code in an append-only manner. Luckily our version control system keeps track of file history and the function can be copy pasted back if the need arises

Adding an API with no consumers or implementors is pretty useless, for added commit history noise.

We have beat this horse to death, There comes a point where discussion has hit a wall, and we hit that wall a while ago. I won't discuss it more. I'll clarify things in a soon-to-be-added md file that describes the review process.

I just propose we upgrade our minimum spec to the level of the ENIAC

If I was in a better mood, I would find this funny. Unfortunately, the response instead frustrates me, as it reads as passive aggressive. You also did not address the point of my message: I don't want us to be factoring magnetometors into our design yet. The concept of a magnetometer should not exist in the codebase (yet). We can start thinking about that later, right now it doesn't need to be present in any imu traits. Lets get the actual architectural design and implementation of a few 6dof imus figured out first.

My current understanding of our collaboration policy:

While I understand you are frustrated, so am I. I have never seen a three person open source project thats only a couple months old need a document detailing the review process. I already spent a lot of time making contribution guidelines mostly focused on tooling, which got out of date very quickly, so I have been hesitant to codify more stuff in READMEs. However, tooling is different from process, so since its important to you to have some clarity on process, I'll write that up in a .md file, submit a PR for it and request your review so you can leave any thoughts.

However, the tone of your message reads to me as very passive aggressive. I'm sure you are well intended and this is borne out of frustration, but please understand that when someone (me) is already quite frustrated with arguing constantly with someone (you), its not going to improve the situation to communicate like you are doing in your comment. I'm sure it took a decent amount of time to filter through discord messages, individually citing each scenario, which isn't really productive either for you or for me.

And when im very frustrated, I become very annoying, which surely saps your mood too

Yes, it does sap my mood. I am having a not good time ™️. At least on github, please be considerate and communicate professionally. I'll also work on being more considerate. Hopefully the tone of my response here is professional and polite. If it reads any other way, this is not my intention, and I'll try to improve on that too.

Jumping through these hoops takes a lot of energy

Right now, no one else is having problems with the hoops. ImUrX and kitlith have been contributing successfully to the codebase fine, the latter to the overlay and the former to the firmware. But I acknowledge that everyone is used to different processes, and for example stuff that works on the overlay may not work in firmware. I'll reach out to them and try to figure out what revisions to the process are necessary to streamline things.

I dont have the same monetary motivators like you do

I have made 50 usd from 1 paypal donation by a person that wanted a specific feature. I've also sponsored @ImUrX with more than 50USD of donations, to cover his hardware expenses. My net operating cost on this project is negative, and I am not currently desgining boards for mass manufacturing (and ferrous_slime is really just for a couple of friends/a very small scale, and I won't be shipping the rust firmware on those boards to them unless we reach a lot more stability). Instead, there are numerous other individuals who will likely have a nrf52840 board production ready well before the firmware is even in a shippable state. Please don't make assumptions about finances and money, its uncouth.

[TheButlah]

Separately from my response btw: I still wanted to know your opinion on this:

So I was wondering if you would be willing to rebase this PR on main and make the changes above. Alternatively, I could proceed with my own implementation if that is more convenient for you.

[ImUrX]

I think a lot of parts have been discussed, but I'm interested on what I was lately been thinking about.

4. Changing one line in an .rs ending file is equally easy as changing one line in a .toml ending file, and former lets us run workflows on all IMUs at once instead of a matrix. This is good because it exposes issues like writing self::imu::ඞ::math, (only the bmi160 integration implements driver crate functionality in firmware). This change would not have been necessary if the pre-required architectural functionalities were implemented first, but you suggested IMUs should be worked on first.

So, I'm kind of against this. But it seems like you trust that It is gonna be better than the toml/env idea, you know what, I want an actual branch trying to do this. I'm interested in seeing how it looks and how well it works. Can you please try implementing this? I will discuss it with you on the discord channel also :P

[Noxime]

Technical:

I meant that we should have two traits, a RawImu and a FusedImu

My using of "this" was confusing. We do have both, but they mean different steps of the data processing pipeline. I intend them as I have implemented them in this PR. FusedImu trait is a source of orientations, that have been somehow fused from IMU data. This can be either done in the firmware through a fusion algorithm like DCM or VQF, or by the chips own firmware. The other trait is Imu, which I was refering to with "this". Imu does not produce quaternions, but rather IMU data. However, the IMU data is not raw data, and thus isnt called RawImu (and ImuData is not called RawImuData). IMU data may be processed in any number of ways. Simplest processing that every Imu implementor does is normalising the values to common metric units. Additionally, filters like lowpassing, throttling, outlier detection, significant motion detection can and should be done before the data is fused to an orientation. Such filtering is either done by our firmware or the IMU firmware

But this PR removes all feature flags for imus, I'm not advocating not merging in imu support, I'm advocating not mixing those architectural changes in with other unrelated features like imu traits

IMU traits (and moving to generics) is a change motivated by wanting to move away from the #[cfg] sus pattern. One does not make sense without the other, and using both is technically impossible. In other words, this PR changed the feature flags because this PR was about getting rid of the feature flags.

Policy:

You are absolutely free to copy any changes you want from my PR to the code. The code is licensed under apache 2.0 and/or MIT, and my ego is not a factor in technical decisions.

Until you can provide me with a concrete target of how you want me to act, I will not be spending my time rebasing my PRs. Once you can clarify how you want contributions to work, in terms that are self-consistent and possible to follow, I will follow that way. I wont spend my free time guessing blindly at what makes you happy.

I am frustrated and unprofessional. Its incredibly frustrating to try and help with a project, when the maintainer gives conflicting requirements and fails to elaborate for weeks on end, or provide any technical or logical reasoning. I apologize for being so petty. I do it in hopes that you acknowledge the framework you make me work in

[TheButlah]

Superseded by #172 #171 #161 #167 #160