Skip to content

PGN testing #411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
acmorrow merged 5 commits into from
Feb 4, 2025
Merged

PGN testing #411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cbe32fa
make test should test all tests in workspace
acmorrow Feb 4, 2025
f6c356c
Use NmeaMessage::try_from at least once in tests
acmorrow Feb 4, 2025
a758b5c
Use PGN constant instead of reiterating
acmorrow Feb 4, 2025
4a28ad8
Fix some clippy warnings we met along the way
acmorrow Feb 4, 2025
1287140
format
acmorrow Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ upload: cargo-ver
cargo +esp espflash flash --package micro-rdk-server --monitor --partition-table micro-rdk-server/esp32/partitions.csv --baud 460800 -f 80mhz --bin micro-rdk-server-esp32 --target=xtensa-esp32-espidf -Zbuild-std=std,panic_abort

test:
cargo test -p micro-rdk --lib --features native,ota
cargo test --workspace --tests --no-fail-fast --features native,ota
Copy link
Contributor

@gvaradarajan gvaradarajan Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is --no-fail-fast?

Copy link
Member Author

@acmorrow acmorrow Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It prevents a failing test from stopping the execution of other tests. I'd much prefer to learn in one go all the tests I broke, rather than it stopping on the first failure.


clippy-native:
cargo clippy -p micro-rdk --no-deps --features native,ota -- -Dwarnings
Expand Down
17 changes: 12 additions & 5 deletions micro-rdk-nmea/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ mod tests {
messages::{
message::Message,
pgns::{
GnssPositionData, GnssSatsInView, TemperatureExtendedRange, WaterDepth,
MESSAGE_DATA_OFFSET,
GnssPositionData, NmeaMessage, NmeaMessageBody, TemperatureExtendedRange,
WaterDepth, MESSAGE_DATA_OFFSET,
},
},
parse_helpers::{
Expand Down Expand Up @@ -157,10 +157,17 @@ mod tests {
let res = general_purpose::STANDARD.decode_vec(msg_str, &mut data);
assert!(res.is_ok());

let cursor = DataCursor::new(data[MESSAGE_DATA_OFFSET..].to_vec());
let message = GnssSatsInView::from_cursor(cursor);
assert!(message.is_ok());
let nmea_message = NmeaMessage::try_from(data);
assert!(nmea_message.is_ok());

let nmea_message = nmea_message.unwrap();
let message = match nmea_message.data {
NmeaMessageBody::GnssSatsInView(val) => Some(val),
_ => None,
};
assert!(message.is_some());
let message = message.unwrap();

Comment on lines +160 to +170
Copy link
Member Author

@acmorrow acmorrow Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gvaradarajan - New since last review, this now ensures that the NmeaMessage enum gets tested at least once.

Copy link
Contributor

@gvaradarajan gvaradarajan Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for adding this

println!("message: {:?}", message);

let source_id = message.source_id();
Expand Down
14 changes: 5 additions & 9 deletions micro-rdk-nmea/src/messages/pgns.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ pub struct GnssSatsInView {
}

macro_rules! define_pgns {
( $(($pgndef:ident, $pgn:expr)),* ) => {
( $($pgndef:ident),* ) => {
Copy link
Member Author

@acmorrow acmorrow Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't push more for this change in #408 because I wasn't completely sure there wasn't a reason it didn't work, but it sure seems to.

Copy link
Contributor

@gvaradarajan gvaradarajan Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh, ok. Sounds good to me

Copy link
Member Author

@acmorrow acmorrow Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the one thing to check is whether this NmeaMessageBody type actually has any tests? If you want, I'll add one.

#[derive(Clone, Debug)]
pub enum NmeaMessageBody {
$($pgndef($pgndef)),*,
Expand All @@ -143,7 +143,7 @@ macro_rules! define_pgns {

pub fn from_bytes(pgn: u32, bytes: Vec<u8>) -> Result<Self, crate::parse_helpers::errors::NmeaParseError> {
Ok(match pgn {
$($pgn => {
$($pgndef::PGN => {
let cursor = DataCursor::new(bytes);
Self::$pgndef($pgndef::from_cursor(cursor)?)
}),*,
Expand All @@ -163,15 +163,11 @@ macro_rules! define_pgns {

pub const MESSAGE_DATA_OFFSET: usize = 32;

define_pgns!(
(WaterDepth, 128267),
(TemperatureExtendedRange, 130316),
(GnssSatsInView, 129540)
);
define_pgns!(WaterDepth, TemperatureExtendedRange, GnssSatsInView);

pub struct NmeaMessage {
metadata: NmeaMessageMetadata,
data: NmeaMessageBody,
pub metadata: NmeaMessageMetadata,
Copy link
Member Author

@acmorrow acmorrow Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed for testing, let me know if you would prefer accessors, but I find them less interesting in a language where immutability is the default.

Copy link
Contributor

@gvaradarajan gvaradarajan Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's just for testing, would pub(crate) work?

Copy link
Member Author

@acmorrow acmorrow Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, I think it would. I always forget that that is an option. Too accustomed to C++ having only public/private. If it works I'll make that change before push.

pub data: NmeaMessageBody,
}

impl TryFrom<Vec<u8>> for NmeaMessage {
Expand Down
1 change: 1 addition & 0 deletions micro-rdk/src/common/conn/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ impl IncomingConnectionManager {
Self { connections }
}

#[allow(dead_code)]
pub(crate) fn max_connections(&self) -> usize {
self.connections.len()
}
Expand Down
1 change: 1 addition & 0 deletions micro-rdk/src/common/robot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,7 @@ impl LocalRobot {
}

pub fn get_periodic_app_client_tasks(&mut self) -> Vec<Box<dyn PeriodicAppClientTask>> {
#[allow(unused_mut)]
Copy link
Contributor

@gvaradarajan gvaradarajan Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes, finally

let mut tasks = Vec::<Box<dyn PeriodicAppClientTask>>::new();

#[cfg(feature = "data")]
Expand Down
3 changes: 1 addition & 2 deletions micro-rdk/src/common/sensor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,7 @@ pub(crate) fn register_models(registry: &mut ComponentRegistry) {
}
}

pub type GenericReadingsResult =
::std::collections::HashMap<::prost::alloc::string::String, google::protobuf::Value>;
pub type GenericReadingsResult = HashMap<::prost::alloc::string::String, google::protobuf::Value>;

#[cfg(feature = "data")]
impl From<GenericReadingsResult> for Data {
Expand Down