Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(Temporary) OTA: change protocolsSupported to single value instead of list #8632

Merged

Conversation

holbrookt
Copy link
Contributor

Problem

List arguments in cluster commands are not yet fully supported, and this means that such commands will fail to be parsed on the receiving end. QueryImage is one of these commands, with the list argument protocolsSupported. In the meantime, we can make protocolsSupported a single value instead of a list in order to unblock QueryImage command receiving.

Change overview

  • Change the command xml definition of QueryImage and protocolsSupported argument
  • Change downstream handler method signatures to accommodate this change

Testing

  • Tested manually with all-clusters-app and chip-tool. Confirmed that QueryImage message is received and parsed successfully

@holbrookt holbrookt added this to the Test Event 5 milestone Jul 26, 2021
@todo
Copy link

todo bot commented Jul 26, 2021

(#8605): protocolsSupported should be list of OTADownloadProtocol enums, not uint8_t

// TODO(#8605): protocolsSupported should be list of OTADownloadProtocol enums, not uint8_t
virtual EmberAfStatus HandleQueryImage(uint16_t vendorId, uint16_t productId, uint16_t imageType, uint16_t hardwareVersion,
uint32_t currentVersion, uint8_t protocolsSupported, const chip::ByteSpan & location,
bool clientCanConsent, const chip::ByteSpan & metadataForProvider) = 0;
virtual EmberAfStatus HandleApplyUpdateRequest(const chip::ByteSpan & updateToken, uint32_t newVersion) = 0;


This comment was generated by todo based on a TODO comment in ce7dfec in #8632. cc @holbrookt.

@todo
Copy link

todo bot commented Jul 26, 2021

(#8605): change this to list */ uint8_t protocolsSupported,

/* TODO(#8605): change this to list */ uint8_t protocolsSupported,
uint8_t * location, uint8_t clientCanConsent,
chip::ByteSpan metadataForProvider)
{
EmberAfStatus status = EMBER_ZCL_STATUS_SUCCESS;
chip::EndpointId endpointId = emberAfCurrentEndpoint();


This comment was generated by todo based on a TODO comment in ce7dfec in #8632. cc @holbrookt.

@todo
Copy link

todo bot commented Jul 26, 2021

(#8605): add array="true" when lists are supported -->

<arg name="protocolsSupported" type="OTADownloadProtocol"/> <!-- TODO(#8605): add array="true" when lists are supported -->
<arg name="location" type="CHAR_STRING" length="2"/>
<arg name="requestorCanConsent" type="BOOLEAN"/>
<arg name="metadataForProvider" type="OCTET_STRING" length="512"/>


This comment was generated by todo based on a TODO comment in ce7dfec in #8632. cc @holbrookt.

@todo
Copy link

todo bot commented Jul 27, 2021

(#8605): add array="true" when lists are supported -->

<arg name="protocolsSupported" type="OTADownloadProtocol"/> <!-- TODO(#8605): add array="true" when lists are supported -->
<arg name="location" type="CHAR_STRING" length="2"/>
<arg name="requestorCanConsent" type="BOOLEAN"/>
<arg name="metadataForProvider" type="OCTET_STRING" length="512"/>


This comment was generated by todo based on a TODO comment in 7b259e4 in #8632. cc @holbrookt.

@todo
Copy link

todo bot commented Jul 28, 2021

(#8605): add array="true" when lists are supported -->

<arg name="protocolsSupported" type="OTADownloadProtocol"/> <!-- TODO(#8605): add array="true" when lists are supported -->
<arg name="location" type="CHAR_STRING" length="2"/>
<arg name="requestorCanConsent" type="BOOLEAN"/>
<arg name="metadataForProvider" type="OCTET_STRING" length="512"/>


This comment was generated by todo based on a TODO comment in 39e7bbe in #8632. cc @holbrookt.

@bzbarsky-apple
Copy link
Contributor

/rebase

@todo
Copy link

todo bot commented Aug 2, 2021

(#8605): add array="true" when lists are supported -->

<arg name="protocolsSupported" type="OTADownloadProtocol"/> <!-- TODO(#8605): add array="true" when lists are supported -->
<arg name="location" type="CHAR_STRING" length="2"/>
<arg name="requestorCanConsent" type="BOOLEAN"/>
<arg name="metadataForProvider" type="OCTET_STRING" length="512"/>


This comment was generated by todo based on a TODO comment in 155d69b in #8632. cc @holbrookt.

@github-actions
Copy link

github-actions bot commented Aug 2, 2021

Size increase report for "esp32-example-build" from 3d8d542

File Section File VM
chip-lock-app.elf .flash.text -64 -64
chip-all-clusters-app.elf .flash.text -36 -36
chip-ipv6only-app.elf .flash.text -172 -172
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lock-app.elf and ./pull_artifact/chip-lock-app.elf:

sections,vmsize,filesize
[Unmapped],0,64
.flash.text,-64,-64

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
[Unmapped],0,36
.debug_str,0,-2
.shstrtab,0,-2
.strtab,0,-2
.debug_frame,0,-12
.flash.text,-36,-36
.debug_info,0,-48
.debug_loc,0,-80
.debug_line,0,-210

Comparing ./master_artifact/chip-ipv6only-app.elf and ./pull_artifact/chip-ipv6only-app.elf:

sections,vmsize,filesize
[Unmapped],0,172
.flash.text,-172,-172

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize


@bzbarsky-apple
Copy link
Contributor

@Damian-Nordic @LuDuda @andy31415 @mspang @msandstedt @saurabhst Please take a look?

@andy31415 andy31415 merged commit 5f3f225 into project-chip:master Aug 4, 2021
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
… list (project-chip#8632)

* change protocolsSupported to single value instead of list

* generate missing zap file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants