Rotating device id representation in TLV Additional Data needs to be aligned with the spec #9455
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
merge from master repo
Update from master
#### Problem Some conversions to use PacketBufferHandle (project-chip#3909) broke Darwin builds, which aren't currently run in CI. #### Summary of Changes Fix src/platform/Darwin/BleConnectionDelegateImpl.mm to match the API change in project-chip#3909.
* New seekbar in Android CHIPTool * Sample usage in lighting-app/nrfconnect Signed-off-by: Markus Becker <[email protected]>
PR project-chip#3704 introduced a change that the BLE transport in RendezvousSession is only initialized when PeerAddress in RendezvousParams is of type BLE. However, PeerAddress isn't initialized anywhere. As a result Rendezvous over BLE stopped working between Android CHIPTool and accessories. Btw, remove an assert related to the storage delegate as it seems an optional member of the device controller.
* Fix data loss or crash in TCPEndPoint with LwIP #### Problem Under the configuration CHIP_SYSTEM_CONFIG_USE_LWIP, in some cases where the data size exceeds the TCP window size, TCPEndPoint can either die or lose data when accounting of un-acked data falls out of sync. #### Summary of Changes Imported fix from Weave: * This change removes separate accounting of the unsent data position and replaces it with simple counting of sent-but-not-acked data and a skip-loop at the start of DriveSending(). Fixes project-chip#4013 - Data loss or crash in TCPEndPoint with LwIP * Restyled by clang-format Co-authored-by: Restyled.io <[email protected]>
…#3966) mDNS doesn't build with no device layer. Remove it from the build and add back the coverage that would catch this that was lost in project-chip#3340.
Looking to remove usage of 'Utils' unless we have really no choice. 'UnitTestRegistration' seems clearer in what it does compared to 'TestUtils'.
* Cleanup zap chip-helper.js * Restyled by clang-format Co-authored-by: Restyled.io <[email protected]>
…#3999) * Move src/transport/tests to auto-test-driver generation * Add relevant libraries (and more test-capable libs) to nrf. * Refactor inet test helpers (to not include actual inet tests), try to make qemu allow better linkage but still failed for transport tests so disabled for now * Added more tests on esp32 qemu * Restyle fixes * Fix cast errors in InetCommon * Disable raw tests from zephyr: somehow they fail running out of endpoints * Disable DNS test on zephyr * Remove inet endpoint test from zephyr * Remove inet endpoint test from zephyr - fix again * Modify gitignore * Restyle fixes
…ject-chip#4017) * Implement the missing part of Exchange Header in Transport layer * Revert comment 'if' back to 'iff'("if and only if")
project-chip#3994) * Remove duplicated send flag defines and put ExchangeMgr/ExchangeContext under the same namespace as CRMP * Rename kSendFlag_Default to kSendFlag_None
…ion (project-chip#3998) * Move src/lib/asn1/tests and src/ble/tests to auto-test-driver generation * Remove one more unused file * Attempt to enable asn1 and ble tests in esp32 - see if they pass or not * Fix merge error * Update include header for ASN1 test * Include ASN1 in libCHIP
* Some conversions to use PacketBufferHandle #### Problem Code should use `PacketBufferHandle` rather than `PacketBuffer *`. #### Summary of Changes - Converts remaining receive path in //src/inet and //src/transport. - Converts most of //src/ble. - Introduces Handle versions of the `AddToEnd`/`DetachTail` pair. Part of issue project-chip#2707 - Figure out a way to express PacketBuffer ownership in the type system * Restyled by clang-format * review * revive BtpEngine::Clear[TR]xPacket() * simplify conditional * (void) message.DetachTail() → message.FreeHead() * remove ExchangeContext::kSendFlag_RetainBuffer * missed pBuf.IsNull() * DetachHead() → PopTail() * typos Co-authored-by: Restyled.io <[email protected]>
* Move src/system/tests to auto-test-driver generation * Remove a TCP/IP init call that was killing qemu
The "all" target exists implicitly and contains everything. We don't need to specify one, and it's misleading to do so specifying deps has no effect.
|
Size increase report for "esp32-example-build" from aff4433
Full report output |
|
Size increase report for "nrfconnect-example-build" from aff4433
Full report output |
jelderton
approved these changes
Sep 15, 2021
examples/chip-tool/commands/payload/AdditionalDataParseCommand.cpp
Outdated
Show resolved
Hide resolved
pullapprove
bot
requested review from
austinh0,
cecille,
Damian-Nordic,
harimau-qirex,
kghost,
msandstedt,
robszewczyk,
turon,
yufengwangca and
yunhanw-google
tcarmelveilleux
approved these changes
Sep 16, 2021
There was a problem hiding this comment.
Approving pending addressing comments in hnnajh@42b1519 (sorry, I mistakenly added the comments in the commit rather than the review)
|
#9804 tracks some remaining problems, including one of my review comments that never got addressed, even though it got marked resolved... |
mleisner
pushed a commit
to mleisner/connectedhomeip
that referenced
this pull request
Sep 20, 2021
…aligned with the spec (project-chip#9455) * RotatingId: version0 * RotatingId: version1 * RotatingId: version0 * RotatingId: version1 * Fix Darwin host build (project-chip#3990) #### Problem Some conversions to use PacketBufferHandle (project-chip#3909) broke Darwin builds, which aren't currently run in CI. #### Summary of Changes Fix src/platform/Darwin/BleConnectionDelegateImpl.mm to match the API change in project-chip#3909. * Add '-Wextra' to compiler flags (project-chip#3902) * Implement Level Control Cluster (project-chip#3806) * New seekbar in Android CHIPTool * Sample usage in lighting-app/nrfconnect Signed-off-by: Markus Becker <[email protected]> * Fix Rendezvous over BLE after recent changes (project-chip#4012) PR project-chip#3704 introduced a change that the BLE transport in RendezvousSession is only initialized when PeerAddress in RendezvousParams is of type BLE. However, PeerAddress isn't initialized anywhere. As a result Rendezvous over BLE stopped working between Android CHIPTool and accessories. Btw, remove an assert related to the storage delegate as it seems an optional member of the device controller. * [thread] fix invalid configuration of active dataset (project-chip#4008) * Fix data loss or crash in TCPEndPoint with LwIP (project-chip#4022) * Fix data loss or crash in TCPEndPoint with LwIP #### Problem Under the configuration CHIP_SYSTEM_CONFIG_USE_LWIP, in some cases where the data size exceeds the TCP window size, TCPEndPoint can either die or lose data when accounting of un-acked data falls out of sync. #### Summary of Changes Imported fix from Weave: * This change removes separate accounting of the unsent data position and replaces it with simple counting of sent-but-not-acked data and a skip-loop at the start of DriveSending(). Fixes project-chip#4013 - Data loss or crash in TCPEndPoint with LwIP * Restyled by clang-format Co-authored-by: Restyled.io <[email protected]> * Update lighting-app gen/ folder with ZAP generated content (project-chip#4010) * Fix segmentation fault error in response echo message (project-chip#3984) * Add back Android default build coverage & fix the build (project-chip#3966) mDNS doesn't build with no device layer. Remove it from the build and add back the coverage that would catch this that was lost in project-chip#3340. * Update all-clusters-app gen/ folder with ZAP generated content (project-chip#3963) * Move src/inet/tests to auto-test-driver generation (project-chip#3997) * Rename TestUtils to UnitTestRegistration. (project-chip#4021) Looking to remove usage of 'Utils' unless we have really no choice. 'UnitTestRegistration' seems clearer in what it does compared to 'TestUtils'. * Update src/lib/core/tests to auto-test-driver generation (project-chip#3991) * Cleanup zap chip-helper.js (project-chip#3973) * Cleanup zap chip-helper.js * Restyled by clang-format Co-authored-by: Restyled.io <[email protected]> * Move src/transport/tests to auto-test-driver generation (project-chip#3999) * Move src/transport/tests to auto-test-driver generation * Add relevant libraries (and more test-capable libs) to nrf. * Refactor inet test helpers (to not include actual inet tests), try to make qemu allow better linkage but still failed for transport tests so disabled for now * Added more tests on esp32 qemu * Restyle fixes * Fix cast errors in InetCommon * Disable raw tests from zephyr: somehow they fail running out of endpoints * Disable DNS test on zephyr * Remove inet endpoint test from zephyr * Remove inet endpoint test from zephyr - fix again * Modify gitignore * Restyle fixes * Use CHIPDeviceController instead of CHIPDeviceController_deprecated (project-chip#3979) * Implement the missing part of Exchange Header in Transport layer (project-chip#4017) * Implement the missing part of Exchange Header in Transport layer * Revert comment 'if' back to 'iff'("if and only if") * Remove duplicated send flag defines and put ExchangeMgr/ExchangeConte… (project-chip#3994) * Remove duplicated send flag defines and put ExchangeMgr/ExchangeContext under the same namespace as CRMP * Rename kSendFlag_Default to kSendFlag_None * Move src/lib/asn1/tests and src/ble/tests to auto-test-driver generation (project-chip#3998) * Move src/lib/asn1/tests and src/ble/tests to auto-test-driver generation * Remove one more unused file * Attempt to enable asn1 and ble tests in esp32 - see if they pass or not * Fix merge error * Update include header for ASN1 test * Include ASN1 in libCHIP * Some conversions to use PacketBufferHandle (project-chip#4011) * Some conversions to use PacketBufferHandle #### Problem Code should use `PacketBufferHandle` rather than `PacketBuffer *`. #### Summary of Changes - Converts remaining receive path in //src/inet and //src/transport. - Converts most of //src/ble. - Introduces Handle versions of the `AddToEnd`/`DetachTail` pair. Part of issue project-chip#2707 - Figure out a way to express PacketBuffer ownership in the type system * Restyled by clang-format * review * revive BtpEngine::Clear[TR]xPacket() * simplify conditional * (void) message.DetachTail() → message.FreeHead() * remove ExchangeContext::kSendFlag_RetainBuffer * missed pBuf.IsNull() * DetachHead() → PopTail() * typos Co-authored-by: Restyled.io <[email protected]> * Move src/system/tests to auto-test-driver generation (project-chip#4000) * Move src/system/tests to auto-test-driver generation * Remove a TCP/IP init call that was killing qemu * Remove explicit "all" target from root build file (project-chip#3967) The "all" target exists implicitly and contains everything. We don't need to specify one, and it's misleading to do so specifying deps has no effect. * Make src/setup_payload compile with -Werror=conversion (project-chip#4032) * Add SSID and password to chip-tool pairing (project-chip#4054) * Get temperature-measurement and all-clusters-app to use examples/common/chip-app-server (project-chip#4039) #### Problem PR project-chip#3704 introduced a change where a `PeerAddress` is now required in order to start `RendezvousSession`. Sadly the multiple code paths bootstrapping `RendezvousSession` has not been updated. PR project-chip#4012 add a fix for some of the `examples/` but not for the `all-clusters-app` nor the `temperature-measurement-app`. To avoid such situation, this PR merge `examples/common/chip-app-server` and the custom code from `all-clusters-app` and `temperature-measurement-app`. One of the more discutable change of this PR (imo) is the code that moves the custom `Echo` mechanism from the `all-clusters-app` to `chip-app-server`. I was hoping to get rid of it before doing this change but the `all-clusters-app` and the `temperature-measurement-app` are broken since project-chip#3704 and this PR should fix that. Also I have a PR (mostly) ready once project-chip#3979 lands to get rid of this `Echo` specific code and replace it by a manufacturer specific `ping` command. #### Summary of Changes * Remove `EchoServer.cpp`, `RendezvousDeviceDelegate.cpp` and `include/RendezvousDeviceDelegate.h` from `all-clusters-app` * Remove `ResponseServer.cpp`, `RendezvousDeviceDelegate.cpp` and `include/RendezvousDeviceDelegate.h` from `temperature-measurement-app` * Introduce `chip-app-server/include/AppDelegate.h` in order to keep the behavior the `all-clusters-app` that turns on/off leds on different events. Maybe it should be converted to some types of `ChipDeviceEvent` or `CHIPCallback` at some point. * Fix `chip-app-server` to accomodate for the specifics of `all-clusters-app` * Add all Thread ULA addresses to the lwip interface (project-chip#4053) ULA prefixes will used for CHIP network so we need to add all these addresses to the interface. * Remove src/lib/message. (project-chip#4055) * Remove src/lib/message. Updated messaging implementation lives in src/messaging and the src/lib/message is not currently compiled or linked in. This saves us from looking at this code when some refactoring is needed (e.g. the SystemPacketBuffer changes). * Remove one more unused file * [ChipTool] Add Payload Parse Command (project-chip#3696) * [ChipTool] Add Payload Parse Command * [ChipTool] Add Payload Parse Command * [ChipTool] Restyle issues resolved * Restyled by whitespace * Resolve build errors caused by Command.h schema change Co-authored-by: lijayaku <[email protected]> Co-authored-by: Restyled.io <[email protected]> * rename ParseCommand to QRCodeParseCommand * adding AdditionalDataParseCommand version 0 * Adding parsing logic + logging * adding another parsing method * Basic Parsing is DONE * fixing memory issue * removing some logs * removing more logs * minor update * Add RotatingDeviceId to DNS-SD * Revert "Merge pull request #4 from hnnajh/rotating-id-test" This reverts commit 0235d05, reversing changes made to 3e1a4b9. * Storing RI in Octet String + Adding Binary format for BLE * Fixing rotating id parser + adding unittests * restyling * refactoring rotating id unit tests * Added more unit tests for Rotating Device Id * updated styling * refactor RI tests * Added RI Unittest + more validation * applying restyling * Fix CI * update styling * Fix CI * Update Styling * Fix CI * Restyling * Fixing nits * Using MutableByteSpan in RI generation * Fixing nits Co-authored-by: Kevin Schoedel <[email protected]> Co-authored-by: Vivien Nicolas <[email protected]> Co-authored-by: Markus Becker <[email protected]> Co-authored-by: Damian Królik <[email protected]> Co-authored-by: Łukasz Duda <[email protected]> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Yufeng Wang <[email protected]> Co-authored-by: Michael Spang <[email protected]> Co-authored-by: Andrei Litvin <[email protected]> Co-authored-by: jepenven-silabs <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]> Co-authored-by: Jiacheng Guo <[email protected]> Co-authored-by: Liju Jayakumar <[email protected]> Co-authored-by: lijayaku <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Per spec, the rotating device id in the TLV Additional Data in the onboarding payload is just a byte blob. The spec currently does not specify how that byte blob is encoded into TLV, but CHIP-Specifications/connectedhomeip-spec#3945 will clarify that it's an octet string
additionalDataPayloadGenerator::generateAdditionalDataPayload currently in the tree does: ReturnErrorOnFailure(writer.OpenContainer(AnonymousTag, kTLVType_Structure, innerWriter)); so it's already doing an anonymous tag, not what the spec used to say.
And the mdns version of rotating device id has a different format from the version in the additional data payload In that the mdns version is hex-encoded and the additional data payload just has binary Per spec, that is. But the implementation doesn't do that right now.
Unit tests are needed cover the generation and parsing of the additional data payload + rotating device id
Solution
Fixed the above problems with the rotating id #9441