From 4bb083116853d0b43253e458ae9331dfcf638c87 Mon Sep 17 00:00:00 2001 From: Kedar Sovani Date: Tue, 8 Aug 2023 15:28:24 +0530 Subject: [PATCH 1/5] BasicInfo: Add ProductName/VendorName/NodeLabel --- examples/onoff_light/src/main.rs | 2 + .../data_model/cluster_basic_information.rs | 63 ++++++++++++++++++- rs-matter/tests/common/im_engine.rs | 2 + rs-matter/tests/data_model/long_reads.rs | 24 ++++++- 4 files changed, 86 insertions(+), 5 deletions(-) diff --git a/examples/onoff_light/src/main.rs b/examples/onoff_light/src/main.rs index 053370a2..de2e1995 100644 --- a/examples/onoff_light/src/main.rs +++ b/examples/onoff_light/src/main.rs @@ -72,6 +72,8 @@ fn run() -> Result<(), Error> { sw_ver_str: "1", serial_no: "aabbccdd", device_name: "OnOff Light", + product_name: "Light123", + vendor_name: "Vendor PQR", }; let (ipv4_addr, ipv6_addr, interface) = initialize_network()?; diff --git a/rs-matter/src/data_model/cluster_basic_information.rs b/rs-matter/src/data_model/cluster_basic_information.rs index dcc70282..ee6279f7 100644 --- a/rs-matter/src/data_model/cluster_basic_information.rs +++ b/rs-matter/src/data_model/cluster_basic_information.rs @@ -15,10 +15,15 @@ * limitations under the License. */ -use core::convert::TryInto; +use core::{cell::RefCell, convert::TryInto}; use super::objects::*; -use crate::{attribute_enum, error::Error, utils::rand::Rand}; +use crate::{ + attribute_enum, + error::{Error, ErrorCode}, + utils::rand::Rand, +}; +use heapless::String; use strum::FromRepr; pub const ID: u32 = 0x0028; @@ -27,8 +32,11 @@ pub const ID: u32 = 0x0028; #[repr(u16)] pub enum Attributes { DMRevision(AttrType) = 0, + VendorName(AttrUtfType) = 1, VendorId(AttrType) = 2, + ProductName(AttrUtfType) = 3, ProductId(AttrType) = 4, + NodeLabel(AttrUtfType) = 5, HwVer(AttrType) = 7, SwVer(AttrType) = 9, SwVerString(AttrUtfType) = 0xa, @@ -39,8 +47,11 @@ attribute_enum!(Attributes); pub enum AttributesDiscriminants { DMRevision = 0, + VendorName = 1, VendorId = 2, + ProductName = 3, ProductId = 4, + NodeLabel = 5, HwVer = 7, SwVer = 9, SwVerString = 0xa, @@ -57,6 +68,8 @@ pub struct BasicInfoConfig<'a> { pub serial_no: &'a str, /// Device name; up to 32 characters pub device_name: &'a str, + pub vendor_name: &'a str, + pub product_name: &'a str, } pub const CLUSTER: Cluster<'static> = Cluster { @@ -70,16 +83,31 @@ pub const CLUSTER: Cluster<'static> = Cluster { Access::RV, Quality::FIXED, ), + Attribute::new( + AttributesDiscriminants::VendorName as u16, + Access::RV, + Quality::FIXED, + ), Attribute::new( AttributesDiscriminants::VendorId as u16, Access::RV, Quality::FIXED, ), + Attribute::new( + AttributesDiscriminants::ProductName as u16, + Access::RV, + Quality::FIXED, + ), Attribute::new( AttributesDiscriminants::ProductId as u16, Access::RV, Quality::FIXED, ), + Attribute::new( + AttributesDiscriminants::NodeLabel as u16, + Access::RWVM, + Quality::N, + ), Attribute::new( AttributesDiscriminants::HwVer as u16, Access::RV, @@ -107,13 +135,16 @@ pub const CLUSTER: Cluster<'static> = Cluster { pub struct BasicInfoCluster<'a> { data_ver: Dataver, cfg: &'a BasicInfoConfig<'a>, + node_label: RefCell>, // Max node-label as per the spec } impl<'a> BasicInfoCluster<'a> { pub fn new(cfg: &'a BasicInfoConfig<'a>, rand: Rand) -> Self { + let node_label = RefCell::new(String::from("")); Self { data_ver: Dataver::new(rand), cfg, + node_label, } } @@ -124,8 +155,13 @@ impl<'a> BasicInfoCluster<'a> { } else { match attr.attr_id.try_into()? { Attributes::DMRevision(codec) => codec.encode(writer, 1), + Attributes::VendorName(codec) => codec.encode(writer, self.cfg.vendor_name), Attributes::VendorId(codec) => codec.encode(writer, self.cfg.vid), + Attributes::ProductName(codec) => codec.encode(writer, self.cfg.product_name), Attributes::ProductId(codec) => codec.encode(writer, self.cfg.pid), + Attributes::NodeLabel(codec) => { + codec.encode(writer, self.node_label.borrow().as_str()) + } Attributes::HwVer(codec) => codec.encode(writer, self.cfg.hw_ver), Attributes::SwVer(codec) => codec.encode(writer, self.cfg.sw_ver), Attributes::SwVerString(codec) => codec.encode(writer, self.cfg.sw_ver_str), @@ -136,12 +172,35 @@ impl<'a> BasicInfoCluster<'a> { Ok(()) } } + + pub fn write(&self, attr: &AttrDetails, data: AttrData) -> Result<(), Error> { + let data = data.with_dataver(self.data_ver.get())?; + + match attr.attr_id.try_into()? { + Attributes::NodeLabel(codec) => { + *self.node_label.borrow_mut() = String::from( + codec + .decode(data) + .map_err(|_| Error::new(ErrorCode::InvalidAction))?, + ); + } + _ => return Err(Error::new(ErrorCode::InvalidAction)), + } + + self.data_ver.changed(); + + Ok(()) + } } impl<'a> Handler for BasicInfoCluster<'a> { fn read(&self, attr: &AttrDetails, encoder: AttrDataEncoder) -> Result<(), Error> { BasicInfoCluster::read(self, attr, encoder) } + + fn write(&self, attr: &AttrDetails, data: AttrData) -> Result<(), Error> { + BasicInfoCluster::write(self, attr, data) + } } impl<'a> NonBlockingHandler for BasicInfoCluster<'a> {} diff --git a/rs-matter/tests/common/im_engine.rs b/rs-matter/tests/common/im_engine.rs index 16e98dc4..02e7847c 100644 --- a/rs-matter/tests/common/im_engine.rs +++ b/rs-matter/tests/common/im_engine.rs @@ -70,6 +70,8 @@ const BASIC_INFO: BasicInfoConfig<'static> = BasicInfoConfig { sw_ver_str: "13", serial_no: "aabbccdd", device_name: "Test Device", + product_name: "TestProd", + vendor_name: "TestVendor", }; struct DummyDevAtt; diff --git a/rs-matter/tests/data_model/long_reads.rs b/rs-matter/tests/data_model/long_reads.rs index 1235f962..333801c4 100644 --- a/rs-matter/tests/data_model/long_reads.rs +++ b/rs-matter/tests/data_model/long_reads.rs @@ -69,18 +69,36 @@ fn wildcard_read_resp(part: u8) -> Vec> { basic_info::AttributesDiscriminants::DMRevision, dont_care.clone() ), + attr_data!( + 0, + 40, + basic_info::AttributesDiscriminants::VendorName, + dont_care.clone() + ), attr_data!( 0, 40, basic_info::AttributesDiscriminants::VendorId, dont_care.clone() ), + attr_data!( + 0, + 40, + basic_info::AttributesDiscriminants::ProductName, + dont_care.clone() + ), attr_data!( 0, 40, basic_info::AttributesDiscriminants::ProductId, dont_care.clone() ), + attr_data!( + 0, + 40, + basic_info::AttributesDiscriminants::NodeLabel, + dont_care.clone() + ), attr_data!( 0, 40, @@ -195,6 +213,9 @@ fn wildcard_read_resp(part: u8) -> Vec> { adm_comm::AttributesDiscriminants::AdminVendorId, dont_care.clone() ), + ]; + + let part2 = vec![ attr_data!(0, 62, GlobalElements::FeatureMap, dont_care.clone()), attr_data!(0, 62, GlobalElements::AttributeList, dont_care.clone()), attr_data!( @@ -203,9 +224,6 @@ fn wildcard_read_resp(part: u8) -> Vec> { noc::AttributesDiscriminants::CurrentFabricIndex, dont_care.clone() ), - ]; - - let part2 = vec![ attr_data!( 0, 62, From 18979feecafd38068b07c3f74fba7722dc2ef58e Mon Sep 17 00:00:00 2001 From: Kedar Sovani Date: Tue, 8 Aug 2023 15:34:33 +0530 Subject: [PATCH 2/5] ACL: Targets in ACL entries are NULLable --- rs-matter/src/acl.rs | 43 ++++++++++++++++++++++++++----------- rs-matter/src/tlv/traits.rs | 16 +++++++++++++- 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/rs-matter/src/acl.rs b/rs-matter/src/acl.rs index 3209c4b1..191a8cec 100644 --- a/rs-matter/src/acl.rs +++ b/rs-matter/src/acl.rs @@ -22,7 +22,7 @@ use crate::{ error::{Error, ErrorCode}, fabric, interaction_model::messages::GenericPath, - tlv::{self, FromTLV, TLVElement, TLVList, TLVWriter, TagType, ToTLV}, + tlv::{self, FromTLV, Nullable, TLVElement, TLVList, TLVWriter, TagType, ToTLV}, transport::session::{Session, SessionMode, MAX_CAT_IDS_PER_NOC}, utils::writebuf::WriteBuf, }; @@ -282,7 +282,15 @@ impl Target { } type Subjects = [Option; SUBJECTS_PER_ENTRY]; -type Targets = [Option; TARGETS_PER_ENTRY]; + +type Targets = Nullable<[Option; TARGETS_PER_ENTRY]>; +impl Targets { + fn init_notnull() -> Self { + const INIT_TARGETS: Option = None; + Nullable::NotNull([INIT_TARGETS; TARGETS_PER_ENTRY]) + } +} + #[derive(ToTLV, FromTLV, Clone, Debug, PartialEq)] #[tlvargs(start = 1)] pub struct AclEntry { @@ -298,14 +306,12 @@ pub struct AclEntry { impl AclEntry { pub fn new(fab_idx: u8, privilege: Privilege, auth_mode: AuthMode) -> Self { const INIT_SUBJECTS: Option = None; - const INIT_TARGETS: Option = None; - Self { fab_idx: Some(fab_idx), privilege, auth_mode, subjects: [INIT_SUBJECTS; SUBJECTS_PER_ENTRY], - targets: [INIT_TARGETS; TARGETS_PER_ENTRY], + targets: Targets::init_notnull(), } } @@ -324,12 +330,20 @@ impl AclEntry { } pub fn add_target(&mut self, target: Target) -> Result<(), Error> { + if self.targets.is_null() { + self.targets = Targets::init_notnull(); + } let index = self .targets + .as_ref() + .notnull() + .unwrap() .iter() .position(|s| s.is_none()) .ok_or(ErrorCode::NoSpace)?; - self.targets[index] = Some(target); + + self.targets.as_mut().notnull().unwrap()[index] = Some(target); + Ok(()) } @@ -358,12 +372,17 @@ impl AclEntry { fn match_access_desc(&self, object: &AccessDesc) -> bool { let mut allow = false; let mut entries_exist = false; - for t in self.targets.iter().flatten() { - entries_exist = true; - if (t.endpoint.is_none() || t.endpoint == object.path.endpoint) - && (t.cluster.is_none() || t.cluster == object.path.cluster) - { - allow = true + match self.targets.as_ref().notnull() { + None => allow = true, // Allow if targets are NULL + Some(targets) => { + for t in targets.iter().flatten() { + entries_exist = true; + if (t.endpoint.is_none() || t.endpoint == object.path.endpoint) + && (t.cluster.is_none() || t.cluster == object.path.cluster) + { + allow = true + } + } } } if !entries_exist { diff --git a/rs-matter/src/tlv/traits.rs b/rs-matter/src/tlv/traits.rs index c013de35..8a7f49a8 100644 --- a/rs-matter/src/tlv/traits.rs +++ b/rs-matter/src/tlv/traits.rs @@ -265,6 +265,20 @@ pub enum Nullable { } impl Nullable { + pub fn as_mut(&mut self) -> Nullable<&mut T> { + match self { + Nullable::Null => Nullable::Null, + Nullable::NotNull(t) => Nullable::NotNull(t), + } + } + + pub fn as_ref(&self) -> Nullable<&T> { + match self { + Nullable::Null => Nullable::Null, + Nullable::NotNull(t) => Nullable::NotNull(t), + } + } + pub fn is_null(&self) -> bool { match self { Nullable::Null => true, @@ -272,7 +286,7 @@ impl Nullable { } } - pub fn unwrap_notnull(self) -> Option { + pub fn notnull(self) -> Option { match self { Nullable::Null => None, Nullable::NotNull(t) => Some(t), From 46ef8ef596d47bf4585efc9a98feed4e27a574d6 Mon Sep 17 00:00:00 2001 From: Kedar Sovani Date: Wed, 9 Aug 2023 07:49:38 +0530 Subject: [PATCH 3/5] ACL: For Writes, perform all ACL checks before any *write* begins --- rs-matter/src/data_model/core.rs | 23 +++++++++++++++++-- rs-matter/tests/data_model/acl_and_dataver.rs | 12 ++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/rs-matter/src/data_model/core.rs b/rs-matter/src/data_model/core.rs index 0a4e99a8..b44da546 100644 --- a/rs-matter/src/data_model/core.rs +++ b/rs-matter/src/data_model/core.rs @@ -28,6 +28,12 @@ use crate::{ // TODO: For now... static SUBS_ID: AtomicU32 = AtomicU32::new(1); +/// The Maximum number of expanded writer request per transaction +/// +/// The write requests are first wildcard-expanded, and these many number of +/// write requests per-transaction will be supported. +pub const MAX_WRITE_ATTRS_IN_ONE_TRANS: usize = 7; + pub struct DataModel(T); impl DataModel { @@ -93,8 +99,21 @@ impl DataModel { ref mut driver, } => { let accessor = driver.accessor()?; - - for item in metadata.node().write(req, &accessor) { + // The spec expects that a single write request like DeleteList + AddItem + // should cause all ACLs of that fabric to be deleted and the new one to be added (Case 1). + // + // This is in conflict with the immediate-effect expectation of ACL: an ACL + // write should instantaneously update the ACL so that immediate next WriteAttribute + // *in the same WriteRequest* should see that effect (Case 2). + // + // As with the C++ SDK, here we do all the ACLs checks first, before any write begins. + // Thus we support the Case1 by doing this. It does come at the cost of maintaining an + // additional list of expanded write requests as we start processing those. + let node = metadata.node(); + let write_attrs: heapless::Vec<_, MAX_WRITE_ATTRS_IN_ONE_TRANS> = + node.write(req, &accessor).collect(); + + for item in write_attrs { AttrDataEncoder::handle_write(&item, &self.0, &mut driver.writer()?) .await?; } diff --git a/rs-matter/tests/data_model/acl_and_dataver.rs b/rs-matter/tests/data_model/acl_and_dataver.rs index 309f127c..18acf303 100644 --- a/rs-matter/tests/data_model/acl_and_dataver.rs +++ b/rs-matter/tests/data_model/acl_and_dataver.rs @@ -371,6 +371,18 @@ fn insufficient_perms_write() { ); } +/// Disabling this test as it conflicts with another part of the spec. +/// +/// The spec expects that a single write request like DeleteList + AddItem +/// should cause all ACLs of that fabric to be deleted and the new one to be added +/// +/// This is in conflict with the immediate-effect expectation of ACL: an ACL +/// write should instantaneously update the ACL so that immediate next WriteAttribute +/// *in the same WriteRequest* should see that effect. +/// +/// This test validates the immediate effect expectation of ACL, but that is disabled +/// since ecosystems routinely send DeleteList+AddItem, so we support that over this. +#[ignore] #[test] /// Ensure that a write to the ACL attribute instantaneously grants permission /// Here we have 2 ACLs, the first (basic_acl) allows access only to the ACL cluster From 0319ece0ab61f9925c051bb262420d758da74ed1 Mon Sep 17 00:00:00 2001 From: Kedar Sovani Date: Wed, 9 Aug 2023 12:25:38 +0530 Subject: [PATCH 4/5] Bump up the stack utilisation --- examples/onoff_light/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/onoff_light/src/main.rs b/examples/onoff_light/src/main.rs index de2e1995..ad72cbd7 100644 --- a/examples/onoff_light/src/main.rs +++ b/examples/onoff_light/src/main.rs @@ -39,7 +39,7 @@ mod dev_att; #[cfg(feature = "std")] fn main() -> Result<(), Error> { let thread = std::thread::Builder::new() - .stack_size(150 * 1024) + .stack_size(160 * 1024) .spawn(run) .unwrap(); From e305ec1d892fd04e2a62641c46b49d98e6a32a16 Mon Sep 17 00:00:00 2001 From: Kedar Sovani Date: Wed, 9 Aug 2023 15:24:33 +0530 Subject: [PATCH 5/5] Bump-up crate version --- rs-matter/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rs-matter/Cargo.toml b/rs-matter/Cargo.toml index c3fc2d14..dac03179 100644 --- a/rs-matter/Cargo.toml +++ b/rs-matter/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "rs-matter" -version = "0.1.0" +version = "0.1.1" edition = "2021" authors = ["Kedar Sovani ", "Ivan Markov", "Project CHIP Authors"] description = "Native Rust implementation of the Matter (Smart-Home) ecosystem"