Skip to content

Comments

async version, and tool to avoid versions getting out of sync#12

Open
alsuren wants to merge 8 commits intoqrasmont:masterfrom
alsuren:async
Open

async version, and tool to avoid versions getting out of sync#12
alsuren wants to merge 8 commits intoqrasmont:masterfrom
alsuren:async

Conversation

@alsuren
Copy link

@alsuren alsuren commented Mar 11, 2025

When implementing the "shared bus" example ( #10 (comment) ), I realised that it would be easier to implement an embedded_hal_async version of bmi2.

What do you think of this general approach?

The idea is that the blocking version can be derived from the async version by deleting code (specifically .await, async , _async and #[allow(async_fn_in_trait)]) and then passing the result through rustfmt.

Potential ways to tidy this up:

a) move the check into a Makefile, so you can type make check to look for differences (then add a make sync to re-generate the blocking code from the async versions)
b) same as a, but using pair of files in scripts/
c) same as a, but using the cargo xtask pattern
d) make the error message a bit better (currently it just relies on the fact that diff will exit nonzero if it spots differences
e) work out how to annotate PRs to put errors on lines that need to change
f) actually just delete interface.rs and bmi2.rs and generate them using a build.rs

I have been working off a hacked version of the esp-hal qa-test/src/bin/embassy_i2c_bmp180_calibration_data.rs example. If you like, I can extract the relevant bits out and add it as an example in this crate.

interesting bits from the example
    static I2C_BUS: StaticCell<I2c0Bus> = StaticCell::new();
    let i2c_bus = I2C_BUS.init(Mutex::new(i2c));

    let mut bmp180_i2c_device = I2cDevice::new(i2c_bus);

    let mut bmi270_i2c_device = I2cDevice::new(i2c_bus);
    let mut bmi = Bmi2::new_i2c(bmi270_i2c_device, I2cAddr::Default, Burst::Other(255));

    // let chip_id = bmi.get_chip_id().unwrap();
    // esp_println::println!("chip id: {chip_id}");

    loop {
        let mut data = [0u8; 22];
        bmp180_i2c_device.write_read(0x76, &[0xaa], &mut data).await.unwrap();
        esp_println::println!("direct:       {:02x?}", data);

        let chip_id = bmi.get_chip_id().await.unwrap();
        esp_println::println!("chip id: {chip_id}");

        Timer::after(Duration::from_millis(1000)).await;
    }

( for context, I am using it in my kitebox firmware, which is using esp32 + embassy https://github.com/hoverkite/hoverkite/tree/main/cross/kitebox )

@alsuren alsuren changed the title WIP: async version, and tool to avoid versions getting out of sync async version, and tool to avoid versions getting out of sync Mar 19, 2025
@qrasmont
Copy link
Owner

Hey! Sorry for the delayed response, I've been sick. I will take some time to look at this in details.

@alsuren
Copy link
Author

alsuren commented Mar 20, 2025

Take it easy and get yourself better. Programming can wait.

In fact, this pr probably shouldn't be merged until I have proven that it works.

I got a bit stuck today trying to debug a FifoExceeded error from esp-hal when I try to run bmi.init(&FILE). Turns out it's this: https://github.com/esp-rs/esp-hal/blob/d53e0b834289c90aaed0829c3d2525e3aa1cc60f/esp-hal/src/i2c/master/mod.rs#L1952-L1958 and setting Burst::Other(31) fixes it. I now have bmi.get_data() telling me a time that is increasing at about 25_000 steps per second, but acc and gyr remain all zeros. I will actually try reading the data sheet and cross referencing some Arduino examples tomorrow.

@JanBerktold
Copy link

Hey there @alsuren, were you able to get the async version to work?

@alsuren
Copy link
Author

alsuren commented Jul 8, 2025

Yes. I managed to get it into the sky (just about):

hoverkite/hoverkite#245

(I should get into the habit of actually merging my prs so people can find the latest progress more easily)

@alsuren
Copy link
Author

alsuren commented Jul 8, 2025

I have recently got a new job so I've put the project down for a bit. I am starting to get into the swing of things though, so I can fix up this PR this week if you want.

If you fundamentally don't like the approach then that's completely fine, and we can close the PR.

@Abrahamh08
Copy link

Is there anything actually missing from this (besides the conflicts with the latest PR)? It looks good and would be pretty useful for me to be able to use in embassy.

@alsuren
Copy link
Author

alsuren commented Sep 12, 2025

Is there anything actually missing from this (besides the conflicts with the latest PR)?

I have only been using it in https://github.com/hoverkite/kitesabre as a joystick of sorts so far. I should really look into the power saving mode at some point, because my m5stack atom is getting pretty warm (might not have anything to do with the bmi2 - I've not checked).

My main concern is the overhead of keeping the async and sync versions in sync with each other. I wouldn't want to inflict that burden on the upstream maintainer, so I spent a bit of time working on that.

I added a Makefile with rules async-to-sync and check-async-sync (naming is hard). The first one will sync any changes from the async version to the sync version, and the second will check for diffs between the two without making any changes (which is what you need for syncing in the other direction)

If you give Claude Sonnet 3.5 the instructions:

run `make check-async-sync` and edit the _async.rs files only until it passes

then it does seem to converge eventually (I doubt it was quicker than doing it myself by hand though)

It looks good and would be pretty useful for me to be able to use in embassy.

Please try using it as a git dependency and tell me how it goes.

bmi2 = { git="https://github.com/alsuren/bmi2", branch="async", features = ["async"] }

@Abrahamh08
Copy link

Abrahamh08 commented Sep 12, 2025

Thanks for updating it. I'll give it a try. I know other drivers duplicate the code (ala your Makefile), but generally it is much easier to use async from sync than the other way around, so in general drivers should be async. But that is up to the maintainer and whether they hold the same opinion and want to make a breaking change.

I'll update on how it goes

@Abrahamh08
Copy link

Abrahamh08 commented Sep 12, 2025

I got it working reading to the point of reading the right chip id at this point. I didn't test past that yet, but the latest two commits on top of the async branch of your fork in my fork are required to make the SPI driver work. It had a few fundamental errors (needing to do a dummy read in the spi constructor, needing to skip a byte for SPI reads, and adding 0x80 to the address for the read bit):
Abrahamh08@aa49723

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants