Merge pull request #85 from kedars/bugfix/ios_fixes_part2

kedars · web-flow · commit bd166e459798 · 2023-08-12T19:11:26.000+05:30
Multiple fixes for working with iOS
diff --git 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();
 
@@ -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/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 <kedars@gmail.com>", "Ivan Markov", "Project CHIP Authors"]
 description = "Native Rust implementation of the Matter (Smart-Home) ecosystem"
diff --git 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<u64>; SUBJECTS_PER_ENTRY];
-type Targets = [Option<Target>; TARGETS_PER_ENTRY];
+
+type Targets = Nullable<[Option<Target>; TARGETS_PER_ENTRY]>;
+impl Targets {
+    fn init_notnull() -> Self {
+        const INIT_TARGETS: Option<Target> = 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<u64> = None;
-        const INIT_TARGETS: Option<Target> = 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/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<u8>) = 0,
+    VendorName(AttrUtfType) = 1,
     VendorId(AttrType<u16>) = 2,
+    ProductName(AttrUtfType) = 3,
     ProductId(AttrType<u16>) = 4,
+    NodeLabel(AttrUtfType) = 5,
     HwVer(AttrType<u16>) = 7,
     SwVer(AttrType<u32>) = 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<String<32>>, // 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/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>(T);
 
 impl<T> DataModel<T> {
@@ -93,8 +99,21 @@ impl<T> DataModel<T> {
                     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/src/tlv/traits.rs b/rs-matter/src/tlv/traits.rs
@@ -265,14 +265,28 @@ pub enum Nullable<T> {
 }
 
 impl<T> Nullable<T> {
+    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,
             Nullable::NotNull(_) => false,
         }
     }
 
-    pub fn unwrap_notnull(self) -> Option<T> {
+    pub fn notnull(self) -> Option<T> {
         match self {
             Nullable::Null => None,
             Nullable::NotNull(t) => Some(t),
diff --git 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/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
diff --git a/rs-matter/tests/data_model/long_reads.rs b/rs-matter/tests/data_model/long_reads.rs
