Skip to content
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.

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

[CONSULT-469] - add more PGN definitions and create sensors #413

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
29 changes: 29 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ jobs:
src-servers: ${{ steps.filter.outputs.src-servers }}
src-cli: ${{ steps.filter.outputs.src-cli }}
src-ffi: ${{ steps.filter.outputs.src-ffi }}
src-nmea: ${{ steps.filter.outputs.src-nmea }}
container:
image: ghcr.io/viamrobotics/micro-rdk-dev-env:amd64
steps:
Expand Down Expand Up @@ -55,6 +56,8 @@ jobs:
- 'micro-rdk-installer/**'
src-ffi:
- 'micro-rdk-ffi/**'
src-nmea:
- 'micro-rdk-nmea/**'

build_cli:
needs: changes
Expand Down Expand Up @@ -105,6 +108,32 @@ jobs:
run: |
bash -c '. /home/testbot/.bash_profile ; git config --global --add safe.directory "$ESP_ROOT"/esp-idf && make clippy-ffi'

check_nmea:
needs: changes
name: Tests, Format, Clippy Micro-RDK NMEA
if: ${{ needs.changes.outputs.src-nmea == 'true'}}
runs-on: ubuntu-latest
container:
image: ghcr.io/viamrobotics/micro-rdk-dev-env:amd64
steps:
- name : Checkout main branch code
if: github.event_name != 'pull_request_target'
uses: actions/checkout@v4
with:
fetch-depth: 2
- name: Check out PR branch code
if: github.event_name == 'pull_request_target'
uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.sha }}
fetch-depth: 2
- name: Format Micro-RDK NMEA
run: |
bash -c 'cargo fmt -p micro-rdk-nmea -- --check'
- name: Clippy Micro-RDK NMEA
run: |
bash -c '. /home/testbot/.bash_profile ; git config --global --add safe.directory "$ESP_ROOT"/esp-idf && make clippy-nmea'

test_and_build:
needs: changes
name: Tests, Format, Clippy Micro-Rdk
Expand Down
3 changes: 3 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ log = "0.4.22"
mdns-sd = { version = "0.12", default-features = false, features = ["async"] }
micro-rdk = { path = "./micro-rdk", default-features = false, features = [] }
micro-rdk-macros = { path = "./micro-rdk-macros" }
micro-rdk-nmea = { path = "./micro-rdk-nmea" }
micro-rdk-nmea-macros = { path = "./micro-rdk-nmea-macros" }
micro-rdk-modular-driver-example = {path = "./examples/modular-drivers" }
once_cell = "1.20.2"
Expand Down
3 changes: 3 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ upload: cargo-ver
test:
cargo test --workspace --tests --no-fail-fast --features native,ota

clippy-nmea:
cargo clippy -p micro-rdk-nmea --no-deps --features native -- -Dwarnings

clippy-native:
cargo clippy -p micro-rdk --no-deps --features native,ota -- -Dwarnings

Expand Down
35 changes: 12 additions & 23 deletions examples/modular-drivers/src/moisture_sensor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,11 @@ use std::{

use micro_rdk::{
common::{
analog::AnalogReaderType,
analog::{AnalogError, AnalogReader, AnalogReaderType},
board::Board,
config::ConfigType,
registry::{get_board_from_dependencies, ComponentRegistry, Dependency, RegistryError},
sensor::{
GenericReadingsResult, Readings, Sensor, SensorError, SensorResult, SensorT,
SensorType, TypedReadingsResult,
},
sensor::{GenericReadingsResult, Readings, Sensor, SensorError, SensorResult, SensorType},
status::{Status, StatusError},
},
google::protobuf,
Expand All @@ -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>> {
Copy link
Member

Choose a reason for hiding this comment

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

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>>;?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, but I was already doing that to Sensor and I wanted to keep the PR small

reader: T,
}

impl MoistureSensor {
impl MoistureSensor<AnalogReaderType<u16>> {
pub fn from_config(_cfg: ConfigType, deps: Vec<Dependency>) -> Result<SensorType, SensorError> {
let board = get_board_from_dependencies(deps);
if board.is_none() {
Expand All @@ -46,28 +43,20 @@ impl MoistureSensor {
}
}

impl Sensor for MoistureSensor {}
impl<T: AnalogReader<u16, Error = AnalogError>> Sensor for MoistureSensor<T> {}

impl Readings for MoistureSensor {
impl<T: AnalogReader<u16, Error = AnalogError>> Readings for MoistureSensor<T> {
fn get_generic_readings(&mut self) -> Result<GenericReadingsResult, SensorError> {
Ok(self
.get_readings()?
.into_iter()
let mut x: HashMap<String, f64> = HashMap::new();
let reading = self.reader.read()?;
x.insert("millivolts".to_string(), reading as f64);
Ok(x.into_iter()
.map(|v| (v.0, SensorResult::<f64> { value: v.1 }.into()))
.collect())
}
}

impl SensorT<f64> for MoistureSensor {
fn get_readings(&self) -> Result<TypedReadingsResult<f64>, SensorError> {
let reading = self.reader.lock().unwrap().read()?;
let mut x = HashMap::new();
x.insert("millivolts".to_string(), reading as f64);
Ok(x)
}
}

impl Status for MoistureSensor {
impl<T: AnalogReader<u16, Error = AnalogError>> Status for MoistureSensor<T> {
fn get_status(&self) -> Result<Option<micro_rdk::google::protobuf::Struct>, StatusError> {
Ok(Some(protobuf::Struct {
fields: HashMap::new(),
Expand Down
4 changes: 4 additions & 0 deletions micro-rdk-ffi/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ publish = false
name = "micrordk"
crate-type = ["staticlib"] # Creates static lib

[features]
viamboat = ["dep:micro-rdk-nmea"]
Copy link
Member

Choose a reason for hiding this comment

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

What is the intended use for this feature. Is this feature gate just to keep the nmea stuff disabled for now and will be removed down the line?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Also we need a better story when we figure out cloud builds on including modules when using micro-rdk as an IDF component


[target.'cfg(not(target_os = "espidf"))'.dependencies]
env_logger.workspace = true
local-ip-address.workspace = true
Expand All @@ -26,6 +29,7 @@ embedded-hal.workspace = true
embedded-svc.workspace = true
futures-lite.workspace = true
micro-rdk = { workspace = true, features = ["esp32", "data", "data-upload-hook-unstable"], default-features = true }
micro-rdk-nmea = { workspace = true, features = ["esp32"], optional = true }
Copy link
Member

Choose a reason for hiding this comment

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

Why is micro-rdk-nmea only a dependency for esp-idf builds?


[dependencies]
base64.workspace = true
Expand Down
12 changes: 9 additions & 3 deletions micro-rdk-ffi/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ fn main() {
.id
.clone();

let viam_modules: Vec<_> = metadata
let viam_modules_filter = metadata
// Obtain the dependency graph from the metadata and iterate its nodes
.resolve
.as_ref()
Expand All @@ -95,8 +95,14 @@ fn main() {
metadata[&dep.pkg].metadata["com"]["viam"]["module"]
.as_bool()
.unwrap_or(false)
})
.collect();
});
// this is because feature-based dependency resolution only occurs after the build script
// runs
#[cfg(not(feature = "viamboat"))]
let viam_modules_filter =
viam_modules_filter.filter(|dep| dep.name.as_str() != "micro_rdk_nmea");
let viam_modules: Vec<_> = viam_modules_filter.collect();

let out_dir = env::var_os("OUT_DIR").expect("OUT_DIR environment variable unset");

let mut modules_rs_content = String::new();
Expand Down
26 changes: 23 additions & 3 deletions micro-rdk-ffi/src/ffi/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use micro_rdk::common::{
exec::Executor,
log::initialize_logger,
provisioning::server::ProvisioningInfo,
registry::{ComponentRegistry, Dependency},
registry::{ComponentRegistry, Dependency, RegistryError},
sensor::{SensorError, SensorType},
webrtc::certificate::Certificate,
};
Expand All @@ -35,6 +35,21 @@ extern "C" {
pub static g_spiram_ok: bool;
}

macro_rules! generate_register_modules {
Copy link
Member

Choose a reason for hiding this comment

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

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?

($($module:ident),*) => {
#[allow(unused_variables)]
fn register_modules(registry: &mut ComponentRegistry) -> Result<(), RegistryError> {
$(
log::info!("registering micro-rdk module '{}'", stringify!($module));
$module::register_models(registry)?;
)*
Ok(())
}
}
}

include!(concat!(env!("OUT_DIR"), "/modules.rs"));

/// Creates a new Viam server context
///
/// Use the returned pointer to register your own components using the C API
Expand All @@ -43,6 +58,8 @@ extern "C" {
#[no_mangle]
pub extern "C" fn init_viam_server_context() -> *mut viam_server_context {
let registry = Box::<ComponentRegistry>::default();
#[cfg(target_os = "espidf")]
initialize_logger::<micro_rdk::esp32::esp_idf_svc::log::EspLogger>();
Copy link
Member

Choose a reason for hiding this comment

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

What prompted moving this here?

Copy link
Member Author

Choose a reason for hiding this comment

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

we missed uploading some logs because we were previously initializing the logger too late

let mut provisioning_info = ProvisioningInfo::default();
provisioning_info.set_manufacturer("viam".to_owned());
provisioning_info.set_model("ffi-provisioning".to_owned());
Expand Down Expand Up @@ -239,7 +256,7 @@ pub unsafe extern "C" fn viam_server_start(ctx: *mut viam_server_context) -> via
return viam_code::VIAM_INVALID_ARG;
}

let ctx = unsafe { Box::from_raw(ctx) };
let mut ctx = unsafe { Box::from_raw(ctx) };

#[cfg(not(target_os = "espidf"))]
{
Expand All @@ -248,7 +265,6 @@ pub unsafe extern "C" fn viam_server_start(ctx: *mut viam_server_context) -> via
#[cfg(target_os = "espidf")]
{
micro_rdk::esp32::esp_idf_svc::sys::link_patches();
initialize_logger::<micro_rdk::esp32::esp_idf_svc::log::EspLogger>();
micro_rdk::esp32::esp_idf_svc::sys::esp!(unsafe {
micro_rdk::esp32::esp_idf_svc::sys::esp_vfs_eventfd_register(
&micro_rdk::esp32::esp_idf_svc::sys::esp_vfs_eventfd_config_t { max_fds: 5 },
Expand Down Expand Up @@ -333,6 +349,10 @@ pub unsafe extern "C" fn viam_server_start(ctx: *mut viam_server_context) -> via
storage
};

if let Err(e) = register_modules(&mut ctx.registry) {
log::error!("couldn't register modules {:?}", e);
}

let mut builder = ViamServerBuilder::new(storage);
builder
.with_provisioning_info(ctx.provisioning_info)
Expand Down
10 changes: 10 additions & 0 deletions micro-rdk-nmea-macros/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ pub(crate) enum UnitConversion {
PascalToBar,
RadianToDegree,
RadPerSecToDegPerSec,
MetersPerSecToKnots,
MetersToNauticalMiles,
}

impl TryFrom<&str> for UnitConversion {
Expand All @@ -26,6 +28,8 @@ impl TryFrom<&str> for UnitConversion {
"C" => Ok(Self::KelvinToCelsius),
"deg" => Ok(Self::RadianToDegree),
"deg/s" => Ok(Self::RadPerSecToDegPerSec),
"knots" => Ok(Self::MetersPerSecToKnots),
"M" => Ok(Self::MetersToNauticalMiles),
x => Err(error_tokens(
format!("encountered unsupported unit {:?}", x).as_str(),
)),
Expand All @@ -48,6 +52,12 @@ impl UnitConversion {
Self::RadianToDegree | Self::RadPerSecToDegPerSec => quote! {
let result = (result as f64) * (180.0 / std::f64::consts::PI);
},
Self::MetersPerSecToKnots => quote! {
let result = (result as f64) * 1.94384;
},
Self::MetersToNauticalMiles => quote! {
let result = (result as f64) * 0.539957;
Copy link
Member

Choose a reason for hiding this comment

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

This conversion looks off to me. One meter isn't ~1/2 a nautical mile. I think this is doing kilometers to nautical miles.

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, my bad

},
}
}
}
Expand Down
10 changes: 9 additions & 1 deletion micro-rdk-nmea/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,17 @@ rust-version.workspace = true

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[features]
esp32 = ['micro-rdk/esp32']
native = ['micro-rdk/native']
Comment on lines +14 to +15
Copy link
Member

Choose a reason for hiding this comment

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

Why does the NMEA library need to know about esp32 vs native?

Copy link
Member Author

Choose a reason for hiding this comment

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

because micro-rdk needs to, see https://viam.atlassian.net/browse/RSDK-9710


[dependencies]
base64 = { workspace = true }
chrono = { workspace = true }
micro-rdk = { workspace = true, features = ["native"] }
log = { workspace = true }
micro-rdk = { workspace = true }
micro-rdk-nmea-macros = { workspace = true }
thiserror = { workspace = true }

[package.metadata.com.viam]
module = true
11 changes: 8 additions & 3 deletions micro-rdk-nmea/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
use micro_rdk::common::registry::{ComponentRegistry, RegistryError};

pub mod messages;
pub mod parse_helpers;
pub mod viamboat;

pub fn register_models(registry: &mut ComponentRegistry) -> Result<(), RegistryError> {
viamboat::register_models(registry)
}

#[cfg(test)]
mod tests {
Expand Down Expand Up @@ -36,9 +43,7 @@ mod tests {
let message = WaterDepth::from_cursor(cursor);
assert!(message.is_ok());
let message = message.unwrap();
let source_id = message.source_id();
assert!(source_id.is_ok());
assert_eq!(source_id.unwrap(), 255);
assert_eq!(message.source_id().unwrap(), 255);
let depth = message.depth();
assert!(depth.is_ok());
assert_eq!(depth.unwrap(), 2.12);
Expand Down
Loading