Generated `OperationalStatus` for `WindowCovering` is wrong

[Luro02]

I am in the process of writing a matter application to control a window covering.

The import!(WindowCovering) macro generates the handler trait, which contains this required method:

pub trait ClusterAsyncHandler {
    // ...

    async fn operational_status(
        &self,
        ctx: &ReadContext<'_>,
    ) -> Result<OperationalStatus, rs_matter::error::Error>;

    // ...
}

The method corresponds to the attribute OperationalStatus with the data type OperationalStatusBitmap in the Matter Application Cluster Specification R1.4.

The specification defines the data type in section 5.3.5.3 as an 8-bit bitmap type where

• the first 2 bits are "global operation state",
• the next two are for "lift operational state"
• and the next two for the "tilt operational state".

Then it defines that for each of the above bitranges how the two bits should be interpreted

• 0b00 = Currently not moving
• 0b01 = Currently opening
• 0b10 = Currently closing

Assuming, I understand this correctly, the specification defines it like this:

enum CurrentMovement {
    NotMoving,
    Opening,
    Closing
}

struct OperationalStatus {
    global: CurrentMovement,
    lift: CurrentMovement,
    tilt: CurrentMovement,
}

Three states that indicate the status of the operations.

---

The definition doesn't specify the interpretations for each of the state bits:

rs-matter/rs-matter-macros/src/idl/parser/controller-clusters.matter

Lines 5484 to 5488 in 2cf6302

	bitmap OperationalStatus : bitmap8 {
	kGlobal = 0x3;
	kLift = 0xC;
	kTilt = 0x30;
	}

That is why the macro then generates something like this:

bitflags::bitflags! {
    struct OperationalStatus: u8 {
        const GLOBAL = 0x03; // 0b0000_0011
        const LIFT   = 0x0C; // 0b0000_1100
        const TILT   = 0x30; // 0b0011_1111
    }
}

which is wrong, 0b11` is not a valid state for any of the operations.
I couldn't find any method to set the individual bits of each of the ranges, which makes this unusable?

I think the definition should look like this:

bitflags::bitflags! {
    struct OperationalStatus: u8 {
        const GLOBAL_OPENING    = 0x01; // 0b0000_0001
        const GLOBAL_CLOSING    = 0x02; // 0b0000_0010

        const LIFT_OPENING      = 0x04; // 0b0000_0100
        const LIFT_CLOSING      = 0x08; // 0b0000_1000

        const TILT_OPENING      = 0x10; // 0b0001_0000
        const TILT_CLOSING      = 0x20; // 0b0010_0000
    }
}

which could then be used like this:

return OperationalStatus::GLOBAL_OPENING | OperationalStatus::LIFT_OPENING;

[ivmarkov]

CC @andreilitvin / @andy31415

I agree with your analysis, and this looks like a bug in the Matter C++ SDK code that generates the IDL from the ZAP JSON "spec".

For one, the latest surface of controller-clusters.matter in the C++ SDK is just as wrong.

What shall we do?

• Option 1: We have [Simplification] Consider replacing the IDL parser with a ZAP JSON parser instead #235, but that would be a lot of lift-and-shift and probably just not worth it doing for that particular hiccup
• Option 2: We can patch in-place (with a special comment so that we don't forget to re-patch once we update the file to a newer version) our own fork of controller-clusters.matter
• Option 3: Work-around the issue by figuring out how to convert your own - manually crafted and correct u8 bitmask - to the OperationalStatus bitflags type (if possible at all)

Option 3 would not require any changes to rs-matter and you can do it externally. Option 2 might require a small PR (BTW PRs welcome!). What would be your preference?

[Luro02]

I think the best solution would be to solve this upstream.

Many of the matter implementations seem to use the "wrong" definition, which makes me wonder whether the matter clients like homeassistant, google home, apple home, ... are implemented correctly (or use the wrong definition as well), and how they interpret it?

At the moment, I do not have a working implementation, so I am unable to test this. From what I can tell, both home assistant and google home request this attribute frequently.

Workaround

I think Option 3 would be the best solution for the immediate future. The bitflags docs mention that unknown bits could be deleted, but based on the generated code, this is not happening with the from_bits_retain method. This makes the following possible:

bitflags::bitflags! {
    struct OperationalStatusNew: u8 {
        const GLOBAL_OPENING    = 0x01; // 0b0000_0001
        const GLOBAL_CLOSING    = 0x02; // 0b0000_0010

        const LIFT_OPENING      = 0x04; // 0b0000_0100
        const LIFT_CLOSING      = 0x08; // 0b0000_1000

        const TILT_OPENING      = 0x10; // 0b0001_0000
        const TILT_CLOSING      = 0x20; // 0b0010_0000
    }
}

bitflags::bitflags! {
    struct OperationalStatus: u8 {
        const GLOBAL = 0x03; // 0b0000_0011
        const LIFT   = 0x0C; // 0b0000_1100
        const TILT   = 0x30; // 0b0011_1111
    }
}

fn assert_bits_not_lost(value: OperationalStatusNew) {
    let other = OperationalStatus::from_bits_retain(value.bits());
    
    assert_eq!(value.bits(), other.bits());
}

fn main() {
    assert_bits_not_lost(OperationalStatusNew::GLOBAL_OPENING);
    assert_bits_not_lost(OperationalStatusNew::LIFT_OPENING);

    assert_bits_not_lost(OperationalStatusNew::GLOBAL_OPENING | OperationalStatusNew::LIFT_OPENING);
    assert_bits_not_lost(OperationalStatusNew::LIFT_OPENING | OperationalStatusNew::TILT_CLOSING);
}

playground

[ivmarkov]

I think the best solution would be to solve this upstream.

Many of the matter implementations seem to use the "wrong" definition, which makes me wonder whether the matter clients like homeassistant, google home, apple home, ... are implemented correctly (or use the wrong definition as well), and how they interpret it?

I'm not so sure they use the "wrong" definition. See here.

The Matter C++ SDK does not have the equivalent of our import! macro, hence they seemingly have hard-coded all their constants, enums and other definitions directly in C++.

It is another topic why we use their IDL as the "source of truth" - given that the very second line inside it says "It is for view/code review purposes only.". Hence #235

Workaround

I think Option 3 would be the best solution for the immediate future. The bitflags docs mention that unknown bits could be deleted, but based on the generated code, this is not happening with the from_bits_retain method.

Alright then! But I'll keep this issue open in case we decide for Option 1 or Option 2 in future.

[andy31415]

the difference is in understanding data types: bitmap vs enum and likely our rs codegen does not. .matter is used as a primary input for java for example, so it is meant to be 100% reliable/correct. parsing XML or JSON will give you the same data, specifically https://github.com/project-chip/connectedhomeip/blob/master/src/app/zap-templates/zcl/data-model/chip/window-covering.xml#L362 says:

  <bitmap name="OperationalStatus" type="bitmap8">
    <cluster code="0x0102"/>
    <field mask="0x03" name="Global"/>
    <field mask="0x0C" name="Lift"/>
    <field mask="0x30" name="Tilt"/>
  </bitmap>

and even the spec parsing at https://github.com/project-chip/connectedhomeip/blob/master/data_model/1.4.2/clusters/WindowCovering.xml#L257 will say it is bitmaps with bitfields.

[andy31415]

If we do chose to parse something, probably the spec XML parsing would be better because that is more accurate than ZAPXML and .matter is a representation of zapxml (there should be no codegen difference between the two).

However overall we probably have to figure out how to generate anything that is bitmapX. It seems like as a minimum we have to handle multi-bit field masks into something nicer ... and then decide if we add to that on top of what our input files provide.

[andy31415]

Spec wise (which is currenlyt NOT parsed anywhere :( ) we have that each of those 2 bits mean:

00b = Currently not moving
01b = Currently opening (e.g. moving from closed to open).
10b = Currently closing (e.g. moving from open to closed).
11b = Reserved

For short term I would suggest to update our import macros to handle bitmap types and provide setters for multi-bit values (is that even possible in a nice way?) and then maybe separately patch for the 3 constants above ... or decide to patch the whole thing in some way.

[ivmarkov]

the difference is in understanding data types: bitmap vs enum and likely our rs codegen does not.

It does differentiate between enums and bitmaps, but apparently, the codegen for bitmaps needs to be a bit more involved.

Currently, how bitmaps are codegen'd is that each variant of the bitmap is converted to a variant of the resulting Rust bitflags! type.

That's fine as long as the IDL (or ZAP) bitmap variant designates a single bit, i.e. a value which is 2^N.

Apparently, not all bitmaps are like that, OperationalStatus being one such example. In the absence of any additional info in the IDL, perhaps we can do a heuristic which - upon detecting an IDL bitmap variant which is not = 2^N we generate multiple bitflags! variants for each bit? In other words, the IDL OperationalStatus::kGlobal = 0x03 would then become - in Rust:

KGLOBAL_BIT1 = 0x01, KGLOBAL_BIT2 = 0x02

...where BIT1 and BIT2 are obviously autogenerated names.

.matter is used as a primary input for java for example, so it is meant to be 100% reliable/correct.

That's great news actually, as the need to do #235 becomes much less pressing. Opened project-chip/connectedhomeip#40587 so that the scary comment can be removed.

parsing XML or JSON will give you the same data, specifically https://github.com/project-chip/connectedhomeip/blob/master/src/app/zap-templates/zcl/data-model/chip/window-covering.xml#L362 says:

and even the spec parsing at https://github.com/project-chip/connectedhomeip/blob/master/data_model/1.4.2/clusters/WindowCovering.xml#L257 will say it is bitmaps with bitfields.

Fair enough. The machine-readable formats are missing the detail we need, so other than doing the heuristics I suggested above, I can't think of any other workaround.

[ivmarkov]

For short term I would suggest to update our import macros to handle bitmap types

As per above, bitmap types are already being handled, except for the logic that checks whether a bitmap variant mask is a multibit one.

and provide setters for multi-bit values (is that even possible in a nice way?) and then maybe separately patch for the 3 constants above ... or decide to patch the whole thing in some way.

I think this should be roughly equivalent to my _BIT1 .. _BITN suffixes anyway, no?

[andy31415]

Agree: we handle bitmaps but not with multi-bit bitfields. BIT1 ... seems good, but user friendliness wise we would want to have the spec text (but not parsed) knowledge what the bits mean.

So I would like to say something like covering.OpState.Global = Closing and have the right bits pop up. Having this work may be harder though, so I would go with bits for now and manually code wrappers/helpers if we really need them.