-
Notifications
You must be signed in to change notification settings - Fork 10
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
New module for integrating Matter into RIOT OS #92
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't go through all the code yet, some initial comments are in here.
matter-rs is a very specific crate (others like embedded-hal or -nal are designed to be interfaces); is there a technical reason why this glue should live in riot-wrappers?
(At any rate, joining multicast groups and having a logger available for applications are additions that fit here for sure).
@@ -56,6 +56,14 @@ embedded-hal-async = { version = "1", optional = true } | |||
|
|||
critical-section = { version = "1.0", optional = true } | |||
|
|||
# for with_matter feature | |||
rs-matter = { git = "https://github.com/maikerlab/rs-matter", branch = "feature/RIOT_OS", default-features = false, features = ["riot-os"], optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it foreseeable that the adjustments there can be upstreamed? Are there open PRs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not yet. I will discuss this with rs-matter contributors, bc it's also not ideal if all the possible specific adaptations for different OS etc. are in this repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked again and I guess we could get rid of all the changes I made in rs-matter.
The only "big" task which needs to be done is implementing embassy-time-driver and embassy-time-queue-driver using ztimer.
src/gnrc/netapi.rs
Outdated
@@ -21,3 +24,11 @@ pub fn dispatch_send( | |||
} | |||
subscribers | |||
} | |||
|
|||
/// Joins an IPV6 multicast group at the provided address | |||
pub fn join_multicast_v6(pid: KernelPID, addr: &[u8; 16]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is sure a more suitable type for an IP address than [u8; 16]; too many to pick from, maybe (given there is gnrc::Address but now also core::net::Ipv6Addr, I'd pick the latter for new code).
The pid parameter could be named scope_id
, that's the term used within Rust for the zone identifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
embedded-nal also has the Ipv6Addr type. Wouldn't it make more sense to use this one? Using core::net::Ipv6Addr, we would have two different types then in riot-wrappers for the same purpose.
If I use embedded-nal in the application code, I have to convert it to core::net::Ipv6Addr just for this function, which is a bit unhandy.
Suggestion: If with_embedded_nal
feature is activated, pub use embedded_nal::Ipv6Addr
, otherwise use the one from core::net?
First I implemented a Matter On/Off Light example application in the main RIOT OS repo, along with all the "glue code" (which really shouldn't be in the application code), so I thought it would be a better idea to have it in riot-wrappers. |
…s activated; handle error in gnrc::netapi::join_multicast_v6
@@ -1,6 +1,10 @@ | |||
//! Components for interacting with IPv6 messages on GNRC | |||
|
|||
use core::mem::MaybeUninit; | |||
#[cfg(any(feature = "with_embedded_nal", feature = "with_embedded_nal_async"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrysn how about that? embedded-nal also uses the types from no-std-net. I don't know if an Ipv6Addr must be used if none of the embedded-nal features are activated, but as a "fallback" we can use the one from core::net
…missioning and device attestation data from VFS
Summary:
matter
module, which contains "glue code" between the rs-matter library and RIOT OSwith_matter
, which will enable thematter
module by activation