eldruin / ads1x1x-rs

Platform-agnostic Rust driver for the ADS1x1x ultra-small, low-power analog-to-digital converters (ADC). Compatible with ADS1013, ADS1014, ADS1015, ADS1113, ADS1114 and ADS1115
https://blog.eldruin.com/ads1x1x-analog-to-digital-converter-driver-in-rust/
Apache License 2.0
31 stars 9 forks source link

Change the address pin system #5

Closed David-OConnor closed 4 years ago

David-OConnor commented 4 years ago

I think this system is easier to understand, from the perspective of the datasheet, from how you wire the device, and from existing guides on the internet. SlaveAddr is now a u8-repr enum with 4 possible values, corresponding to Table 4 of the datasheet.

eldruin commented 4 years ago

I am hesitating on this one. I understand the appeal of this solution. However, I think SlaveAddr::Alternative(...) and SlaveAddr::default() build a more consistent and widespread idiom related to addresses. Nevertheless, I understand it is nice to specify the address just by referring to the physical connection of the pin although this is a slightly different concept and represents more of a builder for an address.

I think I would rather offer helper builder methods for a SlaveAddr like this:

impl SlaveAddr {
    // ...
    pub fn sda() -> Self {
        SlaveAddr::Alternative(true, false)
    }
    pub fn scl() -> Self {
        SlaveAddr::Alternative(true, true)
    }
   // ...
}

// you can now do:
let adc = Ads1x1x::new_ads1013(i2c, SlaveAddr::scl());

Would this be fine for you?

David-OConnor commented 4 years ago

Yep, that would meet the intent. I guess my main point is to be explicit about the 4 possible states of the address as specified in Table 4. The enum restricts it to valid choices, while also exposing the exact addresses without bit arithmetic. Where does the a0, a1 true/false come from?

For example, I don't know how bit arith works, and the terms a0 and a1 are usually associated with the first 2 analog imputs. It's obvious what options are available, how you wire the addr pin to get them, and what the exact address is from this:

#[repr(u8)]
pub enum SlaveAddr {
    /// GND pin: 0x48
    Gnd = 0b100_1000,
    /// VDD pin: 0x49
    Vdd = 0b100_1001,
    /// SDA pin: 0x50
    Sda = 0b100_1010,
    /// SCL pin: 0x51
    Scl = 0b100_1011,
}

Additionally, other libraries I've run across (like this Python one) use the convention of specifying explicit addresses as well.

eldruin commented 4 years ago

I have added the methods new_gnd(), new_vdd(), new_sda() and new_scl() to SlaveAddr and an example to the documentation. I introduced new_ into the name since it is customary to do so. Additionally, I have also slightly improved the documentation of SlaveAddr::Alternative(...) and the example.

I believe this addresses all issues raised here. Thank you for your suggestions/concerns!

David-OConnor commented 4 years ago

Hell yea!

eldruin commented 4 years ago

For the record, I have released the changes in version 0.2.1

David-OConnor commented 4 years ago

Great!