Add support for isFabricScoped on a command & use it in TLSCertificateManagementCluster #37969
Conversation
| {{#if isFabricScoped~}} | ||
| {{#if (isServer source)}} |
There was a problem hiding this comment.
What does it even mean for a server-source command to be fabric-scoped? fabric-scoped in the spec for a command means "command must be associated with a fabric on the recipient of the command", no? Is it actually used for any server-source commands, and what does it mean for those?
There was a problem hiding this comment.
🤷♂️ so in this PR, if you look at TLSCertStruct, it is marked as isFabricScoped, which is then used in FindRootCertificateResponse
I mean if an attribute can be fabric-scoped, I suppose it makes sense that a server-source command can too, right? (aren't they effectively the same, except attributes are pull & server commands are push?)
Here is that fabric scoped struct in spec: https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/f2136a2784837b93efb17814a57f2d4ef29d836a/src/tls/TLSCertificateManagement.adoc#ref_TLSCertStruct
I will defer to you here, I'm just trying to get the spec compiling 🤣
| {{~#if isFabricScoped~}} | ||
| {{#if (isClient source)}} | ||
| CHIP_ERROR Decode(TLV::TLVReader &reader, FabricIndex aAccessingFabricIndex); |
There was a problem hiding this comment.
The relevant thing here, per Slack discussion, is whether the command has structs that are fabric-scoped as command fields, not whether the command itself is fabric-scoped, right? Does anything in the spec actually require a command with a fabric-scoped struct field to itself be fabric-scoped?
There was a problem hiding this comment.
This is really more of a code requirement than a spec requirement - i.e. the command is the root, and so we need a way (at the root) to pass the fabric index to the underlying structs
What we could do (an alternative) would simply be to change all Command::Decode to always take in a aAccessingFabricIndex
I figured the advantage of the approach I've taken here is that it's more explicit, i.e. the caller will be aware that the command has data which is fabric scoped by having to pass in a fabric index
(We could imply from the sub-structs, but at that point we are effectively implicitly marking the command as fabric scoped, so seems reasonable to make it explicit)
src/app/zap-templates/templates/app/im-cluster-command-handler.zapt
Outdated
Show resolved
Hide resolved
| @@ -37,7 +37,12 @@ Protocols::InteractionModel::Status DispatchServerCommand(CommandHandler * apCom | |||
| {{/first}} | |||
| case Commands::{{asUpperCamelCase commandName}}::Id: { | |||
| Commands::{{asUpperCamelCase commandName}}::DecodableType commandData; | |||
|
|
|||
| {{#if isFabricScoped~}} | |||
| TLVError = DataModel::Decode(aDataTlv, commandData, apCommandObj->GetAccessingFabricIndex()); | |||
There was a problem hiding this comment.
I have to ask: do we even need the new DataModel::Decode overload? Why can't we just commandData.Decode(...) here?
There was a problem hiding this comment.
Took a slightly different approach here, adding an overload to Decode method so we don't have to check isFabricScoped here; let me know what you think (i.e. if you prefer calling commandDecode, will make the change
| @@ -859,10 +859,6 @@ CHIP_ERROR DecodableType::Decode(TLV::TLVReader & reader) | |||
| { | |||
| err = DataModel::Decode(reader, identifyTime); | |||
| } | |||
| else | |||
There was a problem hiding this comment.
Sorry about the noise here, just removing these unnecessary empty else {} blocks
| @@ -948,7 +940,7 @@ CHIP_ERROR Type::Encode(TLV::TLVWriter & aWriter, TLV::Tag aTag) const | |||
| return encoder.Finalize(); | |||
| } | |||
|
|
|||
| CHIP_ERROR DecodableType::Decode(TLV::TLVReader & reader) | |||
| CHIP_ERROR DecodableType::Decode(TLV::TLVReader & reader, FabricIndex aAccessingFabricIndex) | |||
There was a problem hiding this comment.
This is correct, i.e. this command is marked isFabricScoped - though not sure if that was intended given that it doesn't have any structs with a fabric index?
There was a problem hiding this comment.
Many many commands are fabric-scoped that have nothing to do with structs at all. All fabric-scoped means for a command is "must be associated with a fabric".
| {{#if isFabricScoped~}} | ||
| {{#if (isServer source)}} |
There was a problem hiding this comment.
🤷♂️ so in this PR, if you look at TLSCertStruct, it is marked as isFabricScoped, which is then used in FindRootCertificateResponse
I mean if an attribute can be fabric-scoped, I suppose it makes sense that a server-source command can too, right? (aren't they effectively the same, except attributes are pull & server commands are push?)
Here is that fabric scoped struct in spec: https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/f2136a2784837b93efb17814a57f2d4ef29d836a/src/tls/TLSCertificateManagement.adoc#ref_TLSCertStruct
I will defer to you here, I'm just trying to get the spec compiling 🤣
| {{~#if isFabricScoped~}} | ||
| {{#if (isClient source)}} | ||
| CHIP_ERROR Decode(TLV::TLVReader &reader, FabricIndex aAccessingFabricIndex); |
There was a problem hiding this comment.
This is really more of a code requirement than a spec requirement - i.e. the command is the root, and so we need a way (at the root) to pass the fabric index to the underlying structs
What we could do (an alternative) would simply be to change all Command::Decode to always take in a aAccessingFabricIndex
I figured the advantage of the approach I've taken here is that it's more explicit, i.e. the caller will be aware that the command has data which is fabric scoped by having to pass in a fabric index
(We could imply from the sub-structs, but at that point we are effectively implicitly marking the command as fabric scoped, so seems reasonable to make it explicit)
src/app/zap-templates/templates/app/im-cluster-command-handler.zapt
Outdated
Show resolved
Hide resolved
| @@ -122,7 +124,7 @@ CHIP_ERROR DataModelLogger::LogCommand(const chip::app::ConcreteCommandPath & pa | |||
| case {{asUpperCamelCase parent.name}}::Commands::{{asUpperCamelCase name}}::Id: | |||
| { | |||
| {{asUpperCamelCase parent.name}}::Commands::{{asUpperCamelCase name}}::DecodableType value; | |||
| ReturnErrorOnFailure(chip::app::DataModel::Decode(*data, value)); | |||
| ReturnErrorOnFailure(chip::app::DataModel::Decode(*data, value, kUndefinedFabricIndex)); | |||
There was a problem hiding this comment.
This is less than ideal... but not sure what else we can do (LogAttribute similarly will log bad data; it's just not obvious because it doesn't have the new method override like commands do)
|
PR #37969: Size comparison from 4d9b423 to 985f8bc Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #37969: Size comparison from 69a4609 to 6043f08 Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Add support for commands marked with
isFabricScopedThe behavior of these commands is specified at decode time if
source=client, with the following method signature:And at encode time if
source=server, with the following updated method signature:This is to guarantee consistent behavior on the receiver of the command (whether client or server), such that the fabric index is always set to by the server.
This is to match attribute behavior.
Testing
Verified by CI