Adc driver

[NoahSprenger]

No description provided.

[BLM16]

@NoahSprenger I'm currently implementing get_XX and set_XX methods for each register. Would it be preferable to implement generic methods more like:

pub fn get<R, T>() -> Result<T>
where
    R: Register
{
    // T is the associated register struct for R, likely found dynamically with a Register -> T map
}

The logic is the same for every register pretty much. Right now the methods are different for the ofcal and fscal as they return a u32, but that u32 could be easily wrapped by a register struct too.

This would be used something like:

adc.get<Register::ID>() // returns an instance of IdRegister

[NoahSprenger]

@NoahSprenger I'm currently implementing get_XX and set_XX methods for each register. Would it be preferable to implement generic methods more like:

pub fn get<R, T>() -> Result<T>
where
    R: Register
{
    // T is the associated register struct for R, likely found dynamically with a Register -> T map
}

The logic is the same for every register pretty much. Right now the methods are different for the ofcal and fscal as they return a u32, but that u32 could be easily wrapped by a register struct too.

This would be used something like:

adc.get<Register::ID>() // returns an instance of IdRegister

Yeah definitely do that to avoid code repetition 👍

[BLM16]

@NoahSprenger I'm currently implementing get_XX and set_XX methods for each register. Would it be preferable to implement generic methods more like:

pub fn get<R, T>() -> Result<T>
where
    R: Register
{
    // T is the associated register struct for R, likely found dynamically with a Register -> T map
}

The logic is the same for every register pretty much. Right now the methods are different for the ofcal and fscal as they return a u32, but that u32 could be easily wrapped by a register struct too.
This would be used something like:

adc.get<Register::ID>() // returns an instance of IdRegister

Yeah definitely do that to avoid code repetition 👍

This won't work unfortunately. I looked into it. Rust does not make individual enum variants a type, thus they cannot be treated generically as individual types and no mapping can be made.

It would be possible to define our own trait or something and make a "Register" trait that implements get and set, and then the Register enum could contain each corresponding trait impl as a value, but this would make for a clunky API.

Let me know what you think. We could also just leave tons of lines of code with redundant getters and setters, or rather than being generic, just write one basic method and pass the Register enum variant as a parameter. This solution just means that we can't guarantee the value being set corresponds to the enum variant as a comptime check.

[NoahSprenger]

This is added since normally one would call a Delay object from the HAL that is clocked by SysTick to delay when needed, but RTIC consumes SysTick. Not feasible (IMO) to propagate RTIC delays down to this level. Possible that an implementation of Delay could be created with a clock object instead of SysTick but I haven't look into this in depth.

[BLM16]

Is delay in μs, and is nop guaranteed to take precisely 1 clock cycle? If nop takes 1 clock cycle, then we'd need to go 0..(delay * cycles/delay_unit_of_time) no?

[BLM16]

I think the default implementations are a good idea and will for sure simplify our API. Would it be worth making all these registers implement a trait that has a default method required? Not sure, but this could simplify the API or enable trait abstraction later on. If you think this is unnecessary (it's probably a bit over the top unless we actually end up needing it), then let's not. Just a thought.

[BLM16]

Careful, this should be 0b0000_0011 because CRC is the last 2 bits.

[BLM16]

0b11 should panic because it's a possible value, not unreachable. Anything that is unreachable (because X & 0b11 will only ever be 0b00..=0b11) is > 0b11. For the values that are possible, it'll make our lives much easier to have a proper panic message should that ever be the result.

See what I've done with the other registers.

Unless you disagree with this. But semantically, unreachable should be impossible mathematically, and 0b11 is totally possible even if it isn't allowed by the ADC. Either way, unreachable will panic if it's hit, but we just lose the nice debug message otherwise.

unreachable_unchecked!() is also an option, and this will not panic if it's hit, the compiler literally will not check this and will treat it as impossible, so it's even more performant, but you thus get nothing at all in the event of an error and the program will rip itself to shreds. We could use it, but only if we really need this performance.

[NoahSprenger]

I think the default implementations are a good idea and will for sure simplify our API. Would it be worth making all these registers implement a trait that has a default method required? Not sure, but this could simplify the API or enable trait abstraction later on. If you think this is unnecessary (it's probably a bit over the top unless we actually end up needing it), then let's not. Just a thought.

I see where you are coming from, but I do believe that would be unnecessary.