[CONSULT-469] - add more PGN definitions and create sensors #413
Conversation
gvaradarajan
commented
Feb 6, 2025
gvaradarajan
commented
- Added missing unit conversions (m/s to knots, meters to nautical miles)
- I've included a feature for inclusion in the micro-rdk-ffi for internal organization reasons
- Implements a variety of sensors using the NMEA parsing logic on raw data including
- Depth Sensor
- Boat Movement Sensor
- Message Sensor (displaying data from specific PGNs)
- Debug Sensor (includes all raw and parsed data)
gvaradarajan
force-pushed
the
micro-rdk-viamboat
branch
from
ccfda1d to
4d915ee
Compare
| @@ -35,6 +35,21 @@ extern "C" { | |||
| pub static g_spiram_ok: bool; | |||
| } | |||
|
|
|||
| macro_rules! generate_register_modules { | |||
There was a problem hiding this comment.
I don't love duplicating this, but I agree we don't have a better way right now. Could you please file a follow-up ticket to revisit the module registration system and hopefully eliminate the duplication of this macro and the build.rs boilerplate as well?
micro-rdk-nmea-macros/src/utils.rs
Outdated
| let result = (result as f64) * 1.94384; | ||
| }, | ||
| Self::MetersToNauticalMiles => quote! { | ||
| let result = (result as f64) * 0.539957; |
There was a problem hiding this comment.
This conversion looks off to me. One meter isn't ~1/2 a nautical mile. I think this is doing kilometers to nautical miles.
| errors::NmeaParseError, | ||
| parsers::{DataCursor, FieldSet, NmeaMessageMetadata}, | ||
| }; | ||
|
|
||
| #[derive(PgnMessageDerive, Clone, Debug)] |
There was a problem hiding this comment.
I know these are, ultimately, going to be generated. However, until they are, I think having them be readable matters.
Can we separate the fields with blank lines:
pub struct Speed {
#[pgn = 128259]
_pgn: PhantomData<u32>,
source_id: u8,
#[scale = 0.01]
#[unit = "knots"]
speed_water_referenced: u16,
#[scale = 0.01]
speed_ground_referenced: u16,
#[lookup]
#[bits = 8]
speed_water_referenced_type: WaterReference,
#[bits = 4]
speed_direction: u8,
}
For this and all the others in this file?
| define_pgns!( | ||
| WaterDepth, | ||
| TemperatureExtendedRange, | ||
| GnssSatsInView, | ||
| CogSog, | ||
| PositionRapidUpdate, | ||
| VesselHeading, | ||
| Attitude, | ||
| Speed, | ||
| DistanceLog | ||
| ); |
There was a problem hiding this comment.
how do you want them sorted?
|
|
||
| pub struct NmeaMessage { | ||
| pub metadata: NmeaMessageMetadata, | ||
| pub data: NmeaMessageBody, | ||
| pub(crate) metadata: NmeaMessageMetadata, |
| esp32 = ['micro-rdk/esp32'] | ||
| native = ['micro-rdk/native'] |
There was a problem hiding this comment.
Why does the NMEA library need to know about esp32 vs native?
There was a problem hiding this comment.
because micro-rdk needs to, see https://viam.atlassian.net/browse/RSDK-9710
| @@ -25,11 +22,11 @@ pub fn register_models(registry: &mut ComponentRegistry) -> Result<(), RegistryE | |||
| } | |||
|
|
|||
| #[derive(DoCommand)] | |||
| pub struct MoistureSensor { | |||
| reader: AnalogReaderType<u16>, | |||
| pub struct MoistureSensor<T: AnalogReader<u16, Error = AnalogError>> { | |||
There was a problem hiding this comment.
IIUC, this is now needed because clippy is worked up. It concludes that it doesn't know if AnalogReaderType is Send because of the dyn AnalogReader.
Would it just be easier to declare AnalogReaderType as pub type AnalogReaderType<W, E = AnalogError> = Arc<Mutex<dyn AnalogReader<W, Error = E> + Send>>;?
There was a problem hiding this comment.
yes, but I was already doing that to Sensor and I wanted to keep the PR small
| raw_pgn_sensor.ok_or(SensorError::ConfigError("missing dependent raw pgn sensor")) | ||
| } | ||
|
|
||
| const DEPTH_PGN: u32 = 128267; |
There was a problem hiding this comment.
Wouldn't it be better to get these PGN's out from the constants? Isn't this just WaterDepth::PGN, which is a CONST so should be useable here?
There was a problem hiding this comment.
Since those structs (and their names) will be auto-generated in the future, the true constant is the PGN
| //! "1FD0A-16": "Cv0BAHg+gD8voKFnAAAAAIdQAwAAAAAACAD/ABYAAgBbAADgeg8A/w==", | ||
| //! "1FD0C-23": "DP0BAHg+gD8voKFnAAAAAL+PAwAAAAAACAD/ACMAAgD/AABQkAT//w==", | ||
| //! "1FD06-16": "Bv0BAHg+gD8voKFnAAAAAFpOAwAAAAAACAD/ABYAAgBb//////YD/w==", | ||
| //! "1FD06-23": "Bv0BAHg+gD8voKFnAAAAAL1FBAAAAAAACAD/ACMAAgD/1HT//////w==", | ||
| //! "1FD07-16": "B/0BAHg+gD8voKFnAAAAANNOBAAAAAAACAD/ABYAAgBb/////3/2Aw==", | ||
| //! "1FD07-23": "B/0BAHg+gD8voKFnAAAAAI9kAwAAAAAACAD/ACMAAgD/wNR0/3///w==" |
There was a problem hiding this comment.
This is a very specific framing of the data. What in the codebase is providing it to us like that? How and why did base64 enter the picture?
| } | ||
| } | ||
|
|
||
| const POSITION_PGN: u32 = 129025; |
