[adc] Add support for HX71708#821
Conversation
EllaFoxo
changed the title
|
Hiya, I'll be coming back to update the docs on this soon, but just want a pair of eyes on the source to make sure it's up to snuff. Cc @garethky |
| # HX717 gain/channel options | ||
| {"A-128": 1}, | ||
| "A-128", | ||
| {320: 4, 3: 80, 20: 2, 10: 1}, |
There was a problem hiding this comment.
looks like the 80 SPS option is transposed
{320: 4, 3: 80, 20: 2, 10: 1},
{320: 4, 80: 3, 20: 2, 10: 1},
| uint_fast8_t gain_or_sps = hx71x->gain_or_sps; | ||
|
|
||
| uint_fast8_t extra_bits; | ||
| if (gain_or_sps == 1) { |
There was a problem hiding this comment.
why not just have a single argument called "extra_bits" or maybe "setting_bits" and used that throughtout. That gets rid of the gain_or_sps flag and sps_bits
Even the error check is checking the same range so it would still function correctly.
| # We forward the sps bits to hx71x_read_adc so that it can read the extra bits required | ||
| if sensor_type == "hx71708": | ||
| self.gain_or_sps = 1 | ||
| self.sps_bits = config.getchoice( |
There was a problem hiding this comment.
This feels like this is being tacked on the side. If there are going to be different configurable properties for different sensor types then we should accept that 100%. It looks like the sub types should not have that responsibility.
Some unique config logic is needed for each sensor type and then the resulting extra_bits get passed in to this class.
For the hx71708 specifically, the gain doesn't need to be read, since its not configurable.
I could argue something similar about the HX711, its SPS cant be set in software. I've already had people complain that they set the SPS but it didn't work. Maybe I need to have HX711_10 and HX711_80 as sensor types so people don't get the wrong idea.
2 participants
The smaller version of the HX717 is the HX71708, which has configurable sample rates instead of the gain/channel, since there is only a single channel on this board. There is a fixed gain value of 128. This adapts the existing HX71x implementation to configure this chip accordingly.
Checklist