Apply fixes as per the code review feedback

Author: ivmarkov

rs-matter/src/dm/clusters/app/color_control.rs

@@ -152,16 +152,16 @@ impl<'a, H: ColorControlHooks> ColorControlHandler<'a, H> {
         }
     }
 
-    /// Apply the `CurrentHueAndCurrentSaturation` mode. Hue is
-    /// stored in `EnhancedCurrentHue` as the low byte.
+    /// Apply the `CurrentHueAndCurrentSaturation` mode. `CurrentHue`
+    /// is the high byte of `EnhancedCurrentHue`.
     fn apply_hue_saturation<N: AttrChangeNotifier>(
         &self,
         ctx: &N,
         hue_u8: u8,
         saturation: u8,
         scene_apply: bool,
     ) {
-        self.hooks.set_enhanced_current_hue(hue_u8 as u16);
+        self.hooks.set_enhanced_current_hue((hue_u8 as u16) << 8);
         self.hooks.set_current_saturation(saturation);
         self.hooks
             .set_enhanced_color_mode(EnhancedColorModeEnum::CurrentHueAndCurrentSaturation);
@@ -417,9 +417,9 @@ impl<H: ColorControlHooks> ColorControlHandler<'_, H> {
                 self.apply_color_temperature(ctx, mireds, true);
             }
             EnhancedColorModeEnum::CurrentHueAndCurrentSaturation => {
-                // Non-enhanced hue is the low byte of EnhancedCurrentHue.
+                // `CurrentHue` is the high byte of `EnhancedCurrentHue`.
                 let (Some(hue), Some(sat)) = (
-                    enhanced_current_hue.map(|h| (h & 0xFF) as u8),
+                    enhanced_current_hue.map(|h| (h >> 8) as u8),
                     current_saturation,
                 ) else {
                     return Ok(());
@@ -851,9 +851,9 @@ mod tests {
 
     #[test]
     fn apply_hue_saturation_mode_truncates_enhanced_hue_to_u8() {
-        // Captured EnhancedCurrentHue is u16; non-enhanced apply
-        // path takes the low byte. Mirrors chip's `ApplyScene`
-        // behaviour and is documented on `apply_hue_saturation`.
+        // `CurrentHue` (u8) is the high byte of `EnhancedCurrentHue`
+        // (u16). Captured EnhancedHue=0x12FF → CurrentHue=0x12 →
+        // round-trip stored as 0x1200.
         let h = handler(Feature::HUE_AND_SATURATION);
         let mut buf = [0u8; 128];
         let len = {
@@ -862,7 +862,6 @@ mod tests {
             let array =
                 AttributeValuePairStructArrayBuilder::new(parent, &crate::tlv::TLVTag::Anonymous)
                     .unwrap();
-            // Hue = 0x12FF → low byte 0xFF after truncation.
             let array = array
                 .push_u16(AttributeId::EnhancedCurrentHue as _, 0x12FF)
                 .unwrap()
@@ -882,10 +881,7 @@ mod tests {
         let avp_list: TLVArray<'_, AttributeValuePairStruct<'_>> = TLVArray::new(elem).unwrap();
         h.apply_inner(NULL_CTX, &avp_list, 0).unwrap();
 
-        // The mutator writes the low byte into the enhanced field
-        // (the cluster's only hue storage); the mode flips to
-        // non-enhanced.
-        assert_eq!(h.hooks.enhanced_current_hue(), 0xFF);
+        assert_eq!(h.hooks.enhanced_current_hue(), 0x1200);
         assert_eq!(h.hooks.current_saturation(), 100);
         assert!(matches!(
             h.hooks.enhanced_color_mode(),

rs-matter/src/dm/clusters/scenes.rs

@@ -63,6 +63,10 @@ const SC_CONSTRAINT_ERROR: u8 = 0x87;
 /// Reserved (invalid) `SceneID` value per Matter Core Spec.
 const RESERVED_SCENE_ID: SceneId = 0xFF;
 
+/// `SceneID` 0 is reserved for the Global Scene; never valid in
+/// add/view/remove/store/recall/copy.
+const GLOBAL_SCENE_ID: SceneId = 0;
+
 /// Maximum legal `AddScene.TransitionTime` in milliseconds
 /// (60 000 seconds / 1000 minutes per Matter Core Spec).
 const MAX_TRANSITION_TIME_MS: u32 = 60_000_000;
@@ -962,7 +966,10 @@ where
 
         // Bad request shape (reserved scene id or oversized
         // transition) takes precedence over the group-table check.
-        if scene_id == RESERVED_SCENE_ID || transition_time > MAX_TRANSITION_TIME_MS {
+        if scene_id == GLOBAL_SCENE_ID
+            || scene_id == RESERVED_SCENE_ID
+            || transition_time > MAX_TRANSITION_TIME_MS
+        {
             return response
                 .status(SC_CONSTRAINT_ERROR)?
                 .group_id(group_id)?
@@ -1012,6 +1019,16 @@ where
             }
         }
 
+        // An oversized EFS payload is a per-scene capacity failure,
+        // surfaced via `SC_INSUFFICIENT_SPACE` (not a transaction error).
+        if raw.len() > M {
+            return response
+                .status(SC_INSUFFICIENT_SPACE)?
+                .group_id(group_id)?
+                .scene_id(scene_id)?
+                .end();
+        }
+
         let status_code = self.state.with(|inner| {
             Self::upsert_scene(
                 inner,
@@ -1112,8 +1129,8 @@ where
         let group_id = request.group_id()?;
         let scene_id = request.scene_id()?;
 
-        // `SceneID = 0xFF` is reserved.
-        if scene_id == RESERVED_SCENE_ID {
+        // `SceneID = 0x00` (Global Scene) and `0xFF` are reserved.
+        if scene_id == GLOBAL_SCENE_ID || scene_id == RESERVED_SCENE_ID {
             return response
                 .status(SC_CONSTRAINT_ERROR)?
                 .group_id(group_id)?
@@ -1198,8 +1215,8 @@ where
         let group_id = request.group_id()?;
         let scene_id = request.scene_id()?;
 
-        // `SceneID = 0xFF` is reserved.
-        if scene_id == RESERVED_SCENE_ID {
+        // `SceneID = 0x00` (Global Scene) and `0xFF` are reserved.
+        if scene_id == GLOBAL_SCENE_ID || scene_id == RESERVED_SCENE_ID {
             return response
                 .status(SC_CONSTRAINT_ERROR)?
                 .group_id(group_id)?
@@ -1292,8 +1309,8 @@ where
         let group_id = request.group_id()?;
         let scene_id = request.scene_id()?;
 
-        // `SceneID = 0xFF` is reserved.
-        if scene_id == RESERVED_SCENE_ID {
+        // `SceneID = 0x00` (Global Scene) and `0xFF` are reserved.
+        if scene_id == GLOBAL_SCENE_ID || scene_id == RESERVED_SCENE_ID {
             return response
                 .status(SC_CONSTRAINT_ERROR)?
                 .group_id(group_id)?
@@ -1397,8 +1414,8 @@ where
         // `FAILURE` and chip-tool's certification suites reject that
         // shape.
 
-        // `SceneID = 0xFF` is reserved.
-        if scene_id == RESERVED_SCENE_ID {
+        // `SceneID = 0x00` (Global Scene) and `0xFF` are reserved.
+        if scene_id == GLOBAL_SCENE_ID || scene_id == RESERVED_SCENE_ID {
             return Err(ErrorCode::ConstraintError.into());
         }
 
@@ -1518,9 +1535,14 @@ where
         // SceneIDs are ignored in this mode).
         let copy_all = (mode.bits() & 0x01) != 0;
 
-        // The reserved `SceneID = 0xFF` is only invalid in
-        // single-scene mode (COPY_ALL ignores those fields).
-        if !copy_all && (scene_from == RESERVED_SCENE_ID || scene_to == RESERVED_SCENE_ID) {
+        // Reserved `SceneID`s (Global Scene `0x00`, `0xFF`) are only
+        // invalid in single-scene mode (COPY_ALL ignores those fields).
+        if !copy_all
+            && (scene_from == GLOBAL_SCENE_ID
+                || scene_from == RESERVED_SCENE_ID
+                || scene_to == GLOBAL_SCENE_ID
+                || scene_to == RESERVED_SCENE_ID)
+        {
             return response
                 .status(SC_CONSTRAINT_ERROR)?
                 .group_identifier_from(group_from)?