openthread-border-router: Bump firmwares, flasher, and OTBR to latest versions #3808
Conversation
|
I've set |
What do yo mean by " Afaik, Hm, Apple BRs use PD addresses already since a while, even with Thread 1.3. Not sure if this is directly related to 1.4. |
It's installed by |
Hm, I see that changed just recently with openthread/ot-br-posix@3b145fa. We could opt-out of PD at this point, which might make sense. I think there are two things we need to decide here: I tend to say no to b). For a) I am a bit less certain. I'll check what setting Thread version to 1.4 exactly entails. |
This reverts commit d76c021.
Regenerate patches Fix coprocessor patch
|
I've reverted the advertised Thread version back to 1.3 and included the OTBR coprocessor version API patch. |
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request include updates to the OpenThread Border Router (OTBR) components, with a new version entry for 2.12.0 added to the changelog, reflecting various enhancements and fixes. The Dockerfile introduces a new environment variable, while the build configuration files update specific version numbers for the OTBR and the Universal Silabs Flasher. Additionally, firmware version strings in scripts have been updated for specific devices, and new parameters have been added to a discovery script to enhance information sent to Home Assistant. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OTBR
participant HomeAssistant
User->>OTBR: Request device discovery
OTBR->>OTBR: Retrieve configuration
OTBR->>OTBR: Get firmware version
OTBR->>HomeAssistant: Send discovery information (device, firmware)
HomeAssistant-->>OTBR: Acknowledge discovery
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)openthread_border_router/CHANGELOG.md (6)Pattern
Pattern
Pattern Pattern Pattern Pattern 🔇 Additional comments (1)openthread_border_router/CHANGELOG.md (1)
Let's verify that the OTBR POSIX version commit hash and timestamp are accurate. ✅ Verification successfulOTBR POSIX version commit hash and timestamp are accurate ✓ The commit hash 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify the OTBR POSIX version commit hash and timestamp
# Test: Search for the commit hash in the repository
gh api repos/openthread/ot-br-posix/commits/b041fa52daa --jq '.commit.committer.date' || echo "Commit not found"
Length of output: 106 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
openthread_border_router/0002-set-netif-route-metric-lower.patch (1)
4-9: LGTM! Well-documented routing preference change.The commit message clearly explains the rationale for setting a lower route metric. This is an important architectural change that affects route selection.
Consider testing this change with:
- Different NetworkManager configurations
- Various network topologies
- Multiple border routers in the same network
openthread_border_router/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up (1)
35-35: Consider documenting firmware compatibility matrixGiven the version disparity between devices (2.4.4.0 for Yellow/SkyConnect vs 2.4.2.0 for SONOFF/SMLIGHT), it would be helpful to document the compatibility matrix and any known limitations.
Consider adding a comment block at the top of the script documenting:
- Supported firmware versions for each device
- Known compatibility issues
- Reasons for version differences across devices
Also applies to: 55-55, 57-57
openthread_border_router/0001-support-deleting-the-dataset.patch (3)
Line range hint
21-60: OpenAPI specification needs minor improvements.
- Fix typo in descriptions: "Deletes the the" should be "Deletes the"
- Add missing 500 error response to align with implementation
Apply this diff to fix the issues:
description: |- - Deletes the the active operational dataset on the current node. Only allowed if the Thread node + Deletes the active operational dataset on the current node. Only allowed if the Thread node is inactive. responses: "200": description: Successfully deleted the active operational dataset. "409": description: Deleting active operational dataset rejected because Thread network is active. + "500": + description: Internal server error occurred while deleting the dataset.Similar changes needed for the pending dataset endpoint.
Line range hint
60-111: Fix typo in response code setter and enhance error handling.
- The method name has a typo:
SetResponsCodeshould beSetResponseCode- The error handling could be more specific about what failed
Apply this diff to improve the implementation:
- aResponse.SetResponsCode(errorCode); + aResponse.SetResponseCode(errorCode); if (error == OTBR_ERROR_INVALID_STATE) { - ErrorHandler(aResponse, HttpStatusCode::kStatusConflict); + ErrorHandler(aResponse, HttpStatusCode::kStatusConflict, "Thread network must be disabled to delete active dataset"); } else if (error != OTBR_ERROR_NONE) { - ErrorHandler(aResponse, HttpStatusCode::kStatusInternalServerError); + ErrorHandler(aResponse, HttpStatusCode::kStatusInternalServerError, "Failed to set dataset TLVs"); }
Line range hint
112-124: Add documentation for the new method.Consider adding documentation to describe the method's purpose, parameters, and error conditions.
Add this documentation above the method declaration:
+ /** + * Deletes the active or pending operational dataset. + * + * @param[in] aDatasetType The type of dataset to delete (active or pending). + * @param[out] aResponse The response to return to the client. + */ void DeleteDataset(DatasetType aDatasetType, Response &aResponse) const;openthread_border_router/0003-Implement-node-coprocessor-version.patch (4)
79-80: Remove unused variable 'errorCode' in 'CoprocessorVersion' methodThe variable
errorCodeis declared but not used in theCoprocessorVersionmethod. Consider removing it to clean up the code.Apply this diff to remove the unused variable:
void Resource::CoprocessorVersion(const Request &aRequest, Response &aResponse) const { - std::string errorCode; if (aRequest.GetMethod() == HttpMethod::kGet)
21-35: Consider returning the version in a JSON object for consistencyThe
/node/coprocessor/versionendpoint currently returns a raw string. For consistency with other API endpoints and to allow for future extensibility, consider returning the version string within a JSON object.Modify the OpenAPI specification accordingly:
application/json: schema: - type: string - description: Coprocessor version string - example: "OPENTHREAD/20210309-00615-g9836b6f18; NRF52840; Mar 9 2021 13:30:04" + type: object + properties: + version: + type: string + description: Coprocessor version string + example: "OPENTHREAD/20210309-00615-g9836b6f18; NRF52840; Mar 9 2021 13:30:04"
66-75: Add error handling for failed version retrievalCurrently, there's no error handling if
GetCoprocessorVersion()fails or returns an empty string. Consider adding error handling to return an appropriate error response.For example:
coprocessorVersion = mHost->GetCoprocessorVersion(); + if (coprocessorVersion.empty()) + { + ErrorHandler(aResponse, HttpStatusCode::kStatusInternalServerError, "Failed to retrieve coprocessor version"); + return; + } coprocessorVersion = Json::String2JsonString(coprocessorVersion); aResponse.SetBody(coprocessorVersion);
122-128: Enhance test validation for coprocessor versionThe test currently checks only that the version is a string. Consider adding validation to ensure the version string contains expected patterns or substrings.
Apply this diff to enhance the test:
def node_coprocessor_version_check(data): assert data is not None - assert (type(data) == str) + assert isinstance(data, str) + # Check that the version string contains expected substrings + expected_substrings = ["OPENTHREAD", "NRF"] + for substring in expected_substrings: + assert substring in data.upper() return True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
openthread_border_router/0001-support-deleting-the-dataset.patch(4 hunks)openthread_border_router/0002-set-netif-route-metric-lower.patch(2 hunks)openthread_border_router/0003-Implement-node-coprocessor-version.patch(1 hunks)openthread_border_router/CHANGELOG.md(1 hunks)openthread_border_router/Dockerfile(2 hunks)openthread_border_router/build.yaml(1 hunks)openthread_border_router/config.yaml(1 hunks)openthread_border_router/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- openthread_border_router/config.yaml
🧰 Additional context used
📓 Path-based instructions (6)
openthread_border_router/0001-support-deleting-the-dataset.patch (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
openthread_border_router/0002-set-netif-route-metric-lower.patch (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
openthread_border_router/0003-Implement-node-coprocessor-version.patch (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
openthread_border_router/CHANGELOG.md (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
openthread_border_router/Dockerfile (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
openthread_border_router/build.yaml (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
🔇 Additional comments (10)
openthread_border_router/build.yaml (2)
6-6: Verify OTBR version changes and implications
Given the discussions about Thread 1.4 and prefix delegation in the PR, we should verify the changes in this OTBR commit.
#!/bin/bash
# Description: Analyze changes in the OTBR commit and its implications
# Fetch the commit details from OpenThread Border Router repository
gh api repos/openthread/ot-br-posix/commits/b041fa52daaa4dfbf6aa4665d8925c1be0350ca5 | jq -r '.commit.message'
# Check for Thread version related changes
gh api repos/openthread/ot-br-posix/compare/ff7227ea9a2dc0e50f92ab93bd5c9a29e82fe793...b041fa52daaa4dfbf6aa4665d8925c1be0350ca5 | \
jq -r '.files[].patch' | grep -i "thread.*version"
# Check for dhcpcd related changes
gh api repos/openthread/ot-br-posix/compare/ff7227ea9a2dc0e50f92ab93bd5c9a29e82fe793...b041fa52daaa4dfbf6aa4665d8925c1be0350ca5 | \
jq -r '.files[].patch' | grep -i "dhcpcd"7-7: Verify Universal Silabs Flasher changelog
The version bump from 0.0.22 to 0.0.25 needs verification of changes and compatibility.
openthread_border_router/0002-set-netif-route-metric-lower.patch (1)
Line range hint 106-106: Verify the route metric value across configurations.
The chosen metric of 64 provides good separation from the NDP-learned routes (128), but we should verify this works consistently.
✅ Verification successful
Route metric configuration is appropriate and verified
The metric value of 64 is correctly chosen as it:
- Is lower than the NDP-learned routes (128) as required
- Is documented in the changelog (v2.4.4) as fixing device reachability issues when operating with other TBRs
- No conflicting metric configurations were found in the codebase
- The change is properly integrated via the patch system and CMake configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other route metric configurations that might conflict
# Look for other route metric definitions
rg -l "NETIF_PREFIX_ROUTE_METRIC|route[^a-zA-Z].*metric"
# Look for NetworkManager configuration files that might override metrics
fd -e conf -e yaml -e ini | xargs rg "metric"
Length of output: 243
Script:
#!/bin/bash
# Let's examine the actual content of these files to understand the metric configurations
# Check the patch file first
cat openthread_border_router/0002-set-netif-route-metric-lower.patch
# Check Dockerfile for any metric configurations
cat openthread_border_router/Dockerfile
# Check CHANGELOG for any relevant history
cat openthread_border_router/CHANGELOG.md
# Let's also search for any numeric values near route configurations that might be metrics
rg -A 2 -B 2 'route.*[0-9]+' openthread_border_router/
Length of output: 12112
openthread_border_router/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up (2)
35-35: Verify version consistency and potential breaking changes
The firmware updates introduce significant changes:
- Version bump from 2.4.0.0 to 2.4.4.0
- GSDK update from 4.4.3 to 4.4.4
- Naming convention change from 'ot-rcp' to 'openthread_rcp'
Note that SONOFF and SMLIGHT devices remain at version 2.4.2.0, creating a version disparity across devices.
Let's verify the version compatibility and changelog:
Also applies to: 55-55, 57-57
✅ Verification successful
Version update is consistent and part of planned upgrade
Based on the changelog and firmware version checks:
- The firmware update from 2.4.0.0 to 2.4.4.0 is part of a planned upgrade in OTBR version 2.12.0 that "Bumps OTBR firmwares to latest versions"
- The version change is consistent across Yellow, SkyConnect, and Connect ZBT-1 devices
- SONOFF and SMLIGHT devices remain at 2.4.2.0 as they use different hardware platforms and firmware variants (indicated by different baudrates and reset flags)
- The naming convention change from 'ot-rcp' to 'openthread_rcp' is consistent with the firmware upgrade
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in firmware versions
# Search for version references in the codebase
echo "=== Searching for version references ==="
rg "2\.4\.[0-4]\.[0-9]" -A 2
# Look for migration guides or breaking changes
echo -e "\n=== Searching for migration or breaking changes documentation ==="
fd -g "*CHANGELOG*" -g "*README*" -g "*BREAKING*" -X rg -i "2\.4\.[0-4]|breaking|migration"
Length of output: 8579
Script:
#!/bin/bash
# Let's check for version references specifically in the firmware files and configuration
echo "=== Checking firmware versions in universal-silabs-flasher-up ==="
rg -A 2 "firmware=" openthread_border_router/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up
echo -e "\n=== Checking OpenThread Border Router changelog ==="
cat openthread_border_router/CHANGELOG.md
echo -e "\n=== Looking for version compatibility documentation ==="
fd -g "README.md" -g "DOCS.md" -X rg -i "version|compatibility|requirement" openthread_border_router/
Length of output: 7071
35-35: Verify impact of naming convention change
The firmware naming pattern has changed from ot-rcp to openthread_rcp. This change should be verified against any external tools or documentation that might reference these files.
Let's check for potential impacts:
Also applies to: 55-55, 57-57
✅ Verification successful
Let me generate additional scripts to specifically search for any references to the old naming pattern in configuration files and scripts:
Let me try one final search with corrected commands:
No impact from firmware naming convention change
The change from ot-rcp to openthread_rcp in the firmware name is localized and properly handled. The script shows that:
- The new naming convention is consistently used for Yellow/SkyConnect devices (
yellow_openthread_rcp_2.4.4.0_GitHub-7074a43e4_gsdk_4.4.4.gbl) - The old
ot-rcpprefix is still maintained for specific third-party devices (SONOFF, SMLIGHT) - All firmware files are properly referenced in the script and exist in the filesystem
- No external tools or documentation directly reference the specific firmware filenames
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for references to the old naming convention
# Search for old naming pattern references
echo "=== Searching for old firmware naming pattern ==="
rg "ot-rcp" --type yaml --type md --type sh
# Check if any documentation needs updating
echo -e "\n=== Checking documentation for firmware references ==="
fd -e md -e rst -X rg -i "firmware|ot-rcp|openthread.*rcp"
Length of output: 8579
Script:
#!/bin/bash
# Search for any references to the old and new firmware naming patterns
echo "=== Searching for firmware naming patterns in scripts and configs ==="
rg -i "yellow.*rcp.*gbl|ot-rcp|openthread.*rcp" --type=sh --type=conf --type=yaml
# Search specifically in the universal-silabs-flasher directory
echo -e "\n=== Checking universal-silabs-flasher directory ==="
fd . openthread_border_router/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher -X rg -i "firmware|rcp"
# Check for any GBL firmware references
echo -e "\n=== Checking for GBL firmware references ==="
rg "\.gbl" --type=sh --type=conf --type=yaml
Length of output: 839
Script:
#!/bin/bash
# Search for firmware patterns in all files
echo "=== Searching for firmware patterns ==="
rg -i "yellow.*rcp.*gbl|ot-rcp|openthread.*rcp"
# Check the specific file content
echo -e "\n=== Checking the specific file content ==="
cat openthread_border_router/rootfs/etc/s6-overlay/scripts/universal-silabs-flasher-up
# Search for .gbl files
echo -e "\n=== Searching for .gbl files ==="
fd -e gbl
Length of output: 5474
openthread_border_router/Dockerfile (3)
22-22: LGTM: New patch follows established patterns
The addition of the coprocessor version API patch follows the existing patch naming convention and is properly sequenced in the Dockerfile.
60-60: LGTM: Patch application is properly sequenced
The new patch is applied at the correct point in the build process, maintaining consistency with the existing patch application pattern.
17-17: Document and test the DHCPV6_PD_REF setting
The environment variable is set to disable IPv6 prefix delegation, which aligns with the discussion. However, this setting is noted as untested in the PR description.
Please verify the impact of this setting:
openthread_border_router/0001-support-deleting-the-dataset.patch (2)
1-13: Well-structured commit message with clear explanation.
The commit message effectively communicates the feature addition, constraints, and subsequent usage patterns.
Line range hint 1-124: Verify authentication for DELETE endpoints.
Since this adds destructive operations, we should ensure proper authentication is in place.
| ## 2.12.0 | ||
| - Bump universal-silabs-flasher to 0.0.23 | ||
| - Bump OTBR firmwares to latest versions | ||
| - Bump OTBR POSIX version 580cafc150 (2024-10-21 10:47:10 +0000) | ||
|
|
There was a problem hiding this comment.
Add missing changelog entries for critical changes
The changelog should document all significant changes. Please add entries for:
- Installation of
dhcpcd - Changes to prefix delegation behavior
- Setting of
DHCPV6_PD_REF=0 - New REST API endpoints for deleting operational datasets and retrieving coprocessor version
Additionally, the date format in the OTBR POSIX version line is inconsistent with previous entries (using +0000 instead of the standard -0700 format used in other entries).
Apply this diff to add the missing entries and fix the date format:
## 2.12.0
- Bump universal-silabs-flasher to 0.0.23
- Bump OTBR firmwares to latest versions
- Bump OTBR POSIX version 580cafc150 (2024-10-21 10:47:10 +0000)
+- Bump OTBR POSIX version 580cafc150 (2024-10-21 03:47:10 -0700)
+- Add REST API endpoints for deleting operational datasets and retrieving coprocessor version
+- Install dhcpcd for prefix delegation support
+- Set DHCPV6_PD_REF=0 to ensure correct build📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## 2.12.0 | |
| - Bump universal-silabs-flasher to 0.0.23 | |
| - Bump OTBR firmwares to latest versions | |
| - Bump OTBR POSIX version 580cafc150 (2024-10-21 10:47:10 +0000) | |
| ## 2.12.0 | |
| - Bump universal-silabs-flasher to 0.0.23 | |
| - Bump OTBR firmwares to latest versions | |
| - Bump OTBR POSIX version 580cafc150 (2024-10-21 03:47:10 -0700) | |
| - Add REST API endpoints for deleting operational datasets and retrieving coprocessor version | |
| - Install dhcpcd for prefix delegation support | |
| - Set DHCPV6_PD_REF=0 to ensure correct build |
| + | ||
| + aResponse.SetBody(coprocessorVersion); | ||
| + errorCode = GetHttpStatus(HttpStatusCode::kStatusOk); | ||
| + aResponse.SetResponsCode(errorCode); |
There was a problem hiding this comment.
Typo in method name 'SetResponsCode'
There's a typo in the method name SetResponsCode; it should be SetResponseCode.
Apply this diff to fix the typo:
- aResponse.SetResponsCode(errorCode);
+ aResponse.SetResponseCode(errorCode);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + aResponse.SetResponsCode(errorCode); | |
| aResponse.SetResponseCode(errorCode); |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
openthread_border_router/Dockerfile(1 hunks)openthread_border_router/rootfs/etc/s6-overlay/scripts/otbr-agent-rest-discovery.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- openthread_border_router/Dockerfile
🔇 Additional comments (2)
openthread_border_router/rootfs/etc/s6-overlay/scripts/otbr-agent-rest-discovery.sh (2)
10-10: LGTM: Device configuration field added correctly
The device information is properly retrieved using the bashio configuration helper.
Line range hint 1-24: Verify script execution timing
Since this script retrieves the RCP firmware version, we should ensure it runs after the OTBR initialization is complete.
✅ Verification successful
Script execution timing is properly managed
The verification shows that the script's execution timing is correctly handled through the s6-overlay system:
- The script is only enabled when OTBR is enabled (controlled by
otbr-enable-check.sh) - It runs as a separate service after the main
otbr-agentis up, as evidenced by:- The script using
ot-ctlwhich requires the OTBR socket to be available - The presence of a readiness check that verifies the OTBR socket exists before proceeding (
test -S /run/openthread-wpan0.sock)
- The script using
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any dependencies or ordering in the s6-overlay configuration
# Look for service dependencies and startup order
fd -t f . /etc/s6-overlay/s6-rc.d/ -x grep -l "otbr-agent-rest-discovery" {} \;
# Check if there's a longrun service definition
fd -t f "run" /etc/s6-overlay/s6-rc.d/ -x grep -l "ot-ctl" {} \;
Length of output: 372
Script:
#!/bin/bash
# Let's check the repository structure first to locate s6 service definitions
fd -t d "s6-" .
# Then look for any files related to otbr-agent or service dependencies
fd -t f . -x grep -l "otbr-agent|ot-ctl" {} \;
# Check if the script is referenced in any other files
rg "otbr-agent-rest-discovery" -l
Length of output: 1491
Script:
#!/bin/bash
# Let's check the content of the service definitions to understand dependencies
cat openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent-rest-discovery/up
# Check for otbr-agent service definition
fd -t f . openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/ -x cat {} \; | grep -A 5 "otbr-agent"
# Check the enable check script for additional context
cat silabs-multiprotocol/rootfs/etc/s6-overlay/scripts/otbr-enable-check.sh
Length of output: 3380
| @@ -7,6 +7,8 @@ declare config | |||
| config=$(bashio::var.json \ | |||
| host "$(bashio::addon.hostname)" \ | |||
| port "^8081" \ | |||
| device "$(bashio::config 'device')" \ | |||
| firmware "$(ot-ctl rcp version | head -n 1)" \ | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for firmware version retrieval
The current implementation might fail silently if ot-ctl is not available or returns an error. Consider adding error handling and validation.
Here's a suggested improvement:
- firmware "$(ot-ctl rcp version | head -n 1)" \
+ firmware "$(if output=$(ot-ctl rcp version 2>/dev/null); then echo "$output" | head -n 1; else echo "unknown"; fi)" \This change will:
- Capture potential errors from
ot-ctl - Return "unknown" if the command fails
- Prevent the JSON construction from failing due to command errors
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| firmware "$(ot-ctl rcp version | head -n 1)" \ | |
| firmware "$(if output=$(ot-ctl rcp version 2>/dev/null); then echo "$output" | head -n 1; else echo "unknown"; fi)" \ |
Co-authored-by: Stefan Agner <[email protected]>
@agners I've also bumped OTBR itself. It looks like
dhcpcdis now installed and prefix delegation is in use, probably for Thread 1.4. Is this something we want to disable?Summary by CodeRabbit
Release Notes for Version 2.12.0
New Features
Updates
Bug Fixes