RSDK-7149 - implement resolution for analogs and remove get_board_status #207
Conversation
| @@ -71,6 +71,11 @@ pub mod proto { | |||
| include!("gen/viam.app.datasync.v1.rs"); | |||
| } | |||
| } | |||
| pub mod mltraining { | |||
| pub mod v1 { | |||
| include!("gen/viam.app.mltraining.v1.rs"); | |||
There was a problem hiding this comment.
I believe it effectively became mandatory to gen mltraining for anything consuming app as a consequence of viamrobotics/api@f02f6cc#diff-e842ca0f50219c67d40be90a2a1efd40ee1b6df19a254357ab177d631b3ea4b4R5
micro-rdk/src/common/analog.rs
Outdated
|
|
||
| #[derive(Debug, Default)] | ||
| pub struct AnalogResolution { | ||
| pub min_range: f64, |
There was a problem hiding this comment.
if the proto takes f32 let's leave it at f32
| // all units are in mV | ||
| AnalogResolution { | ||
| min_range: 150.0, | ||
| max_range: 2450.0, |
There was a problem hiding this comment.
true for the linear range for the ADC can actually measure higher than 2.45mv might make more sense to use esp_adc_cal_raw_to_voltage to feed min max and resolution (0-4095) into this struct.
There was a problem hiding this comment.
it's far from perfect since adc curve are not linear for error will accumulate especially outside the 150-2450 range
There was a problem hiding this comment.
sorry, I'm a little unclear here. That function seems to directly compute voltage, so do you mean I should replace the raw value with the value in mV? If so does it actually make sense to even return non-trivial values for the resolution?
There was a problem hiding this comment.
The range in this struct is the suggested voltage range for esp32 for accurate voltage measurement where raw to volatge is mostly linear. That doesn't mean ADC is not able to measure voltage outside of this range. I don't know if the purpose of this api is to offload the conversion to SDKs but if it is the case then we should calcultage mon/max voltage by calling esp_adc_cal_raw_to_voltage(0,&chars) for min and esp_adc_cal_raw_to_voltage(4095,&chars) for min/max (can use the rust bindings for that too) and assume it's linear for the range (which is not) Would need to confirm the values are not too insane
Ideally we should return mV instead of raw
There was a problem hiding this comment.
I agree with the last sentence, maybe I return the mV value and make the step_size 1?
There was a problem hiding this comment.
if the returned value goes above 2450 what's happening on the SDK side?
| fn resolution(&self) -> AnalogResolution { | ||
| Default::default() | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug, Default)] |
There was a problem hiding this comment.
do we want to be returning derived Default values here? We'd always be returning a min_range, max_range, and step_size of 0.0.
Would it not be better to return some "realistic" defaults that we can test the FakeAnalogReader against later?
There was a problem hiding this comment.
yeah, sure I could throw in some numbers I guess
There was a problem hiding this comment.
LGTM I am glad board status gets removed :P I see in the proto file there is still
message Status {
map<string, int32> analogs = 1;
map<string, int64> digital_interrupts = 2;
}Do we know what's the purpose? Do we need to implement it
Lastly how do we validate behavior? IRC Board Status was just returning Analog readers and DigitalInterrupt status (which we didn't implement?)
I checked RDK and that message was being used only internally and gets turned into a generic struct field for I figured asking for |
gvaradarajan
force-pushed
the
RSDK-7149-breaking-proto-changes
branch
from
f7cd5a3
to
2a60708
Compare
|
Tested on hardware |
When auto-generating the proto code, the removal of
BoardStatusfrom the Board API also made its way in. Separating it out felt harder than simply removing the board status specific code on our side.