Interest in supporting async?

[jamesmunns]

Hey there! I have a project where I'll need to use an ADS1110 inside of an embassy based project.

I'll certainly base my work off of this crate, but I'm not sure the best way of supporting both sync and async approaches. From a first guess, I'd likely keep all the constants and register definitions shared, but have two separate types and I/O impls for blocking and async interfaces.

I'll plan to do this async conversion in a fork to start off, but if I get it to a reasonable place, would you be interested in having me upstream those changes?

Thanks!

[eldruin]

Hi James, yes, I would certainly welcome an async version here.
I am not sure what the best way would be, though.

It might be possible to keep the same Ads1x1x type and just have different implementations with different trait bounds for blocking/async requiring either the current WriteData/... traits or some new WriteDataAsync or similar.
If that does not work, you could also add a generic marker type and switch on that.

The ugly thing is that basically the whole code is duplicated. Do you have any experience with maybe-async-cfg?

What would be nice would be to keep one central Ads1x1x type for both blocking or async.

[jamesmunns]

Hey!

So, in respect of "how" to do this, I also added async support to the MAX31855 crate in this PR, where I isolated the I/O and non-I/O part, so that everything could be shared. This is a much simpler driver than the current ads1x1x, but maybe enough to see the "vibe" of how I'd recommend doing it. The public interface is duplicated, but most of the "guts" are shared, outside of how you read/write the actual data.

However, I also realized three things after working on this today:

• The ADS1x1x crate has a lot of infrastructure/types/generics for handling different variants and their different details
• The ADS1110 isn't a currently supported chip by ADS1x1x
• The ADS1110 is very simple, and doesn't need most of the machinery that the ads1x1x provides

Since I'm doing this for a client project, it seemed to make the most sense to implement the minimal functionality I needed, which I've done here.

Happy to discuss upstreaming these bits tho, as well as the maybe-async approach.

[jamesmunns]

In particular, it seems like the ADS1110 has a pretty different 8-bit (instead of 16-bit) configuration register, which doesn't look like it is compatible. So actually abstracting over these differences might not even be reasonable. Comparing:

struct Register;
impl Register {
    const CONVERSION: u8 = 0x00;
    const CONFIG: u8 = 0x01;
    const LOW_TH: u8 = 0x02;
    const HIGH_TH: u8 = 0x03;
}

struct BitFlags;
impl BitFlags {
    const OS: u16 = 0b1000_0000_0000_0000;
    const MUX2: u16 = 0b0100_0000_0000_0000;
    const MUX1: u16 = 0b0010_0000_0000_0000;
    const MUX0: u16 = 0b0001_0000_0000_0000;
    const PGA2: u16 = 0b0000_1000_0000_0000;
    const PGA1: u16 = 0b0000_0100_0000_0000;
    const PGA0: u16 = 0b0000_0010_0000_0000;
    const OP_MODE: u16 = 0b0000_0001_0000_0000;
    const DR2: u16 = 0b0000_0000_1000_0000;
    const DR1: u16 = 0b0000_0000_0100_0000;
    const DR0: u16 = 0b0000_0000_0010_0000;
    const COMP_MODE: u16 = 0b0000_0000_0001_0000;
    const COMP_POL: u16 = 0b0000_0000_0000_1000;
    const COMP_LAT: u16 = 0b0000_0000_0000_0100;
    const COMP_QUE1: u16 = 0b0000_0000_0000_0010;
    const COMP_QUE0: u16 = 0b0000_0000_0000_0001;
}

With:

The ADS1110 doesn't even have "registers", you just write 8-bits to change the settings, read 16 bits to get the last ADC conversion value, or read 24 bits to get ADC value + settings.

[eldruin]

I see. Yes, I think it does not make much sense to abstract over this.
We could merge support for the ADS1110 into this crate via a marker type and trait which switch the implementations, similar to what I do to add features only for the chips that support it here.
One could define another Tier1 or better-named trait for the currently common functionality and gate the methods on it, so that it is otherwise empty.
Then, one could add another Ads1110 or better-named trait only implemented for an ic::Ads1110 marker type, which enables your separate implementation but where the methods are similarly named.
For the user it would be a pretty smooth experience, where they only choose the chip with the construction method and everything else is hidden.
However, I understand it is a bit complicated. What would be somewhat ugly would be the need for Ads1110-specific data-rate and full-scale range types.

[Jasperlooney]

I am also looking to use ads1115 with Embassy. The crate works without Embassy but the use of Embassy introduces a lifetime ('static) which causes this issue.

Below is the code block and then the resulting build error. I hope that this provides some help.

CODE:

#[main]
async fn main(spawner: Spawner) {
    static SIGNAL: StaticCell<Signal<NoopRawMutex, [u8; 32]>> = StaticCell::new();
    let signal = &*SIGNAL.init(Signal::new());
    esp_println::println!("Init!");
    let peripherals = Peripherals::take();
    let system = SystemControl::new(peripherals.SYSTEM);
    let clocks = ClockControl::boot_defaults(system.clock_control).freeze();

    let timg0 = TimerGroup::new_async(peripherals.TIMG0, &clocks);
    esp_hal_embassy::init(&clocks, timg0);

    const ADDRESS: u8 = 0x48;
    let io = Io::new(peripherals.GPIO, peripherals.IO_MUX);

    // Default pins for Uart/Serial communication
    //let tx_pin = io.pins.gpio21;
    //let rx_pin = io.pins.gpio20;
    let pins = TxRxPins::new_tx_rx(io.pins.gpio21, io.pins.gpio20);

    let config = Config::default();
    config
        .baudrate(115_200)
        .data_bits(DataBits::DataBits8)
        .stop_bits(StopBits::STOP1)
        .parity_none();
    //config.rx_fifo_full_threshold(READ_BUF_SIZE as u16);
    //


    let i2c_var = I2C::new( 
        peripherals.I2C0,
        io.pins.gpio4,
        io.pins.gpio5,
        400.kHz(),
        &clocks,
        None
    );


    let c = ads1x1x::Ads1x1x::new_ads1115(i2c_var, ads1x1x::SlaveAddr::Default);

ERROR:

error[E0277]: the trait bound `esp_hal::i2c::I2C<'_, I2C0, Blocking>: embedded_hal::blocking::i2c::Write` is not satisfied
   --> src/main.rs:174:43
    |
174 |     let c = ads1x1x::Ads1x1x::new_ads1115(i2c_var, ads1x1x::SlaveAddr::Default);
    |             ----------------------------- ^^^^^^^ the trait `embedded_hal::blocking::i2c::Write` is not implemented for `esp_hal::i2c::I2C<'_, I2C0, Blocking>`
    |             |
    |             required by a bound introduced by this call
    |
note: required by a bound in `ads1x1x::construction::<impl Ads1x1x<I2cInterface<I2C>, Ads1115, Resolution16Bit, OneShot>>::new_ads1115`
   --> /home/****/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ads1x1x-0.2.2/src/construction.rs:46:1
    |
46  | impl_new_destroy!(Ads1115, new_ads1115, destroy_ads1115, ic::Resolution16Bit);
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    | |                          |
    | |                          required by a bound in this associated function
    | required by this bound in `ads1x1x::construction::<impl Ads1x1x<I2cInterface<I2C>, Ads1115, Resolution16Bit, OneShot>>::new_ads1115`
    = note: this error originates in the macro `impl_new_destroy` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `esp_hal::i2c::I2C<'_, I2C0, Blocking>: embedded_hal::blocking::i2c::WriteRead` is not satisfied
   --> src/main.rs:174:43
    |
174 |     let c = ads1x1x::Ads1x1x::new_ads1115(i2c_var, ads1x1x::SlaveAddr::Default);
    |             ----------------------------- ^^^^^^^ the trait `embedded_hal::blocking::i2c::WriteRead` is not implemented for `esp_hal::i2c::I2C<'_, I2C0, Blocking>`
    |             |
    |             required by a bound introduced by this call
    |
note: required by a bound in `ads1x1x::construction::<impl Ads1x1x<I2cInterface<I2C>, Ads1115, Resolution16Bit, OneShot>>::new_ads1115`
   --> /home/****/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ads1x1x-0.2.2/src/construction.rs:46:1
    |
46  | impl_new_destroy!(Ads1115, new_ads1115, destroy_ads1115, ic::Resolution16Bit);
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    | |                          |
    | |                          required by a bound in this associated function
    | required by this bound in `ads1x1x::construction::<impl Ads1x1x<I2cInterface<I2C>, Ads1115, Resolution16Bit, OneShot>>::new_ads1115`
    = note: this error originates in the macro `impl_new_destroy` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0277`.

Apologies for commenting on an issue from over a year ago, I don't know if this counts as necro posting.

If there is more information I can give please let me know.

[eldruin]

@Jasperlooney That is an unrelated issue possibly caused by your esp-hal version using embedded-hal 1.0 but this crate not using that version yet. You should be able to fix it by using embedded-hal-compat.

[Jasperlooney]

@eldruin Thank you so much! I have been wrestling with this issue for about a month. The code compiles now and you are an absolute legend. Thank you for addressing my issue even though it turned out to be unrelated.