Skip to content

Conversation

@SimonFrost-Arm
Copy link
Contributor

Support for updated (breaking) change in CCA top level token made to conform with updated CMW definition.
Change to spec can be found in RMM 1.1-alp14.

… new-cca-top-tokenSupport for updated (breaking) change in CCA top level token made to conform with updated CMW definition.

Change to spec can be found in RMM 1.1-alp14.
@yogeshbdeshpande
Copy link
Contributor

The changes LGTM, just one question:

I noticed the RMM spec referred in the README.md still uses
cca-token = #6.399(cca-token-collection) ;

Is there a plan to change the spec? as currently the spec seems out of sync with the proposed changes.

@SimonFrost-Arm
Copy link
Contributor Author

Is there a plan to change the spec? as currently the spec seems out of sync with the proposed changes.
The change will be in the upcoming 1.1alp14

Copy link
Contributor

@yogeshbdeshpande yogeshbdeshpande left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@setrofim setrofim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash the commits, add Signed-off-by footer, and update the commit message to align with Conventional Commits.

Apart from that, LGTM.

@thomas-fossati
Copy link
Contributor

thomas-fossati commented May 2, 2025

I think we can be a bit more conservative on the decoding side.
The fact that the CBOR tags are different allows us to support the EAT collection in parallel with the CMW collection pretty straightforwardly.

Something like:

 var (
        tags = []encoding.CBORTagEntry{
                {Type: CBORCollection{}, Tag: 399},
                {Type: CMWCollection{}, Tag: 903},
        }

and then

type CMWCollection struct {
	PlatformTokenColl CMWCollectionEntry `cbor:"44234,keyasint"`
	RealmTokenColl    CMWCollectionEntry `cbor:"44241,keyasint"`

Or do that manually, by first decoding the top-level tag, and then dispatching to the right collection decoder:

  var t cbor.RawTag

  if err = t.UnmarshalCBOR(b); err != nil {
          return fmt.Errorf("unmarshal top-level CBOR Tag: %w", err)
  }
  
  switch t.Number {
  case 903: // do new
  case 399: // do legacy
  }

This would give senders the ability to align with the new format at their own pace.

…onform with updated CMW definition.

Change to spec can be found in RMM 1.1-alp14.
Code will still decode EAT Collection formatted tokens

Signed-off-by: simfro01 <[email protected]>
…updated CMW definition.

Change to spec can be found in RMM 1.1-alp14.
Change supports new token format and will still decode previous EAT Collection format

Signed-off-by: simfro01 <[email protected]>
@SimonFrost-Arm
Copy link
Contributor Author

Latest commit adds support for reading old (399) tokens as well as the full CMW tokens

@yogeshbdeshpande
Copy link
Contributor

Latest commit adds support for reading old (399) tokens as well as the full CMW tokens

Apologies for a late reply, but wanted to check, is this complexity really required, at this stage supporting two types of collection ?

The products using token type are not yet out in the field, so if we can unilaterally change that is a better fit for now ?
Just my two cents!

@thomas-fossati
Copy link
Contributor

Latest commit adds support for reading old (399) tokens as well as the full CMW tokens

Apologies for a late reply, but wanted to check, is this complexity really required, at this stage supporting two types of collection ?

Yes, there is running code, in testing labs and in CIs, that’s better not brutally break :-)

…updated CMW definition.

Change to spec can be found in RMM 1.1-alp14.
Change supports new token format and will still decode previous EAT Collection format

Signed-off-by: simfro01 <[email protected]>
…updated CMW definition.

Change to spec can be found in RMM 1.1-alp14.
Change supports new token format and will still decode previous EAT Collection format
@SimonFrost-Arm
Copy link
Contributor Author

Merged separately to cleanup messy commit history

@SimonFrost-Arm SimonFrost-Arm deleted the new-cca-top-token branch May 12, 2025 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants