Skip to content

[radio] introduce secured Enh-ACK properties to RxFrameProperties - #13361

Merged
jwhui merged 1 commit into
openthread:mainfrom
abtink:radio/missing-rx-prop
Jul 20, 2026
Merged

[radio] introduce secured Enh-ACK properties to RxFrameProperties#13361
jwhui merged 1 commit into
openthread:mainfrom
abtink:radio/missing-rx-prop

Conversation

@abtink

@abtink abtink commented Jul 17, 2026

Copy link
Copy Markdown
Member

This commit introduces IsAckedWithSecEnhAck(), GetAckFrameCounter(), and GetAckKeyIndex() helper methods in RxFrameProperties class to encapsulate raw received Enh-ACK security information.

This commit also updates the remaining direct struct member accesses of mInfo.mTxInfo across data_poll_sender.cpp, sub_mac.cpp, and radio_platform.cpp to use the matching TxFrameProperties helper methods.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request improves the codebase by enforcing better encapsulation of radio frame properties. By introducing dedicated helper methods for accessing secured Enh-ACK information and standardizing access to transmission properties, the changes reduce direct dependencies on internal struct members, leading to cleaner and more maintainable code.

Highlights

  • Encapsulation of Security Information: Introduced helper methods IsAckedWithSecEnhAck(), GetAckFrameCounter(), and GetAckKeyIndex() in RxFrameProperties to encapsulate raw Enh-ACK security data.
  • Code Cleanup: Updated multiple modules including data_poll_sender.cpp, sub_mac.cpp, and radio_platform.cpp to replace direct struct member access with established helper methods for better abstraction.
New Features

🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors direct accesses to internal frame fields (such as transmission delay, retransmission flags, and ACK properties) across several MAC and radio files, replacing them with cleaner getter and setter helper methods. A critical issue was identified in SubMac::BeginTransmit(), where calling the new GetTxDelay, SetTxDelay, and SetTxDelayBaseTime methods unconditionally will cause compilation failures when building for Thread 1.1 or lower; wrapping this block in a preprocessor check is recommended.

Comment thread src/core/mac/sub_mac.cpp
@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown

Merging #13361 into main

name branch text data bss total
ot-cli-ftd c7d4a87 481496 860 66532 548888
c5dde51 481496 860 66532 548888
+/- +0 +0 +0 +0
ot-ncp-ftd c7d4a87 446948 764 61768 509480
c5dde51 446948 764 61768 509480
+/- +0 +0 +0 +0
ot-cli-mtd c7d4a87 373752 764 50932 425448
c5dde51 373752 764 50932 425448
+/- +0 +0 +0 +0
ot-ncp-mtd c7d4a87 355172 764 46200 402136
c5dde51 355172 764 46200 402136
+/- +0 +0 +0 +0
ot-cli-ftd-br c7d4a87 592952 864 130684 724500
c5dde51 592952 864 130684 724500
+/- +0 +0 +0 +0
ot-rcp c7d4a87 64016 568 20828 85412
c5dde51 64016 568 20828 85412
+/- +0 +0 +0 +0
Library files
name branch text data bss total
libopenthread-ftd.a c7d4a87 251472 95 40311 291878
c5dde51 251472 95 40311 291878
+/- +0 +0 +0 +0
libopenthread-cli-ftd.a c7d4a87 61777 0 8091 69868
c5dde51 61777 0 8091 69868
+/- +0 +0 +0 +0
libopenthread-ncp-ftd.a c7d4a87 33359 0 5948 39307
c5dde51 33359 0 5948 39307
+/- +0 +0 +0 +0
libopenthread-mtd.a c7d4a87 171027 0 24743 195770
c5dde51 171027 0 24743 195770
+/- +0 +0 +0 +0
libopenthread-cli-mtd.a c7d4a87 41482 0 8059 49541
c5dde51 41482 0 8059 49541
+/- +0 +0 +0 +0
libopenthread-ncp-mtd.a c7d4a87 25783 0 5948 31731
c5dde51 25783 0 5948 31731
+/- +0 +0 +0 +0
libopenthread-ftd-br.a c7d4a87 363626 99 104423 468148
c5dde51 363626 99 104423 468148
+/- +0 +0 +0 +0
libopenthread-cli-ftd-br.a c7d4a87 80892 0 8131 89023
c5dde51 80892 0 8131 89023
+/- +0 +0 +0 +0
libopenthread-rcp.a c7d4a87 9946 0 5060 15006
c5dde51 9946 0 5060 15006
+/- +0 +0 +0 +0
libopenthread-radio.a c7d4a87 20781 0 262 21043
c5dde51 20781 0 262 21043
+/- +0 +0 +0 +0

This commit introduces `IsAckedWithSecEnhAck()`, `GetAckFrameCounter()`,
and `GetAckKeyIndex()` helper methods in `RxFrameProperties` class to
encapsulate raw received Enh-ACK security information.

This commit also updates the remaining direct struct member accesses of
`mInfo.mTxInfo` across `data_poll_sender.cpp`, `sub_mac.cpp`, and
`radio_platform.cpp` to use the matching `TxFrameProperties` helper
methods.
@abtink
abtink force-pushed the radio/missing-rx-prop branch from 363a372 to 99c9ca0 Compare July 17, 2026 21:20
@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 69.23077% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.20%. Comparing base (c7d4a87) to head (99c9ca0).

Files with missing lines Patch % Lines
src/core/mac/sub_mac.cpp 66.66% 2 Missing ⚠️
src/core/radio/radio_platform.cpp 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13361      +/-   ##
==========================================
- Coverage   75.17%   70.20%   -4.98%     
==========================================
  Files         710      710              
  Lines       95168    96492    +1324     
==========================================
- Hits        71540    67739    -3801     
- Misses      23628    28753    +5125     
Files with missing lines Coverage Δ
src/core/mac/data_poll_sender.cpp 92.59% <100.00%> (ø)
src/core/mac/sub_mac_csl_receiver.cpp 81.44% <100.00%> (ø)
src/core/radio/radio_frame.hpp 100.00% <100.00%> (ø)
src/core/mac/sub_mac.cpp 79.42% <66.66%> (-11.97%) ⬇️
src/core/radio/radio_platform.cpp 38.83% <0.00%> (-13.60%) ⬇️

... and 176 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@abtink
abtink marked this pull request as ready for review July 17, 2026 23:24
@abtink

abtink commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors direct member accesses to frame properties with cleaner getter and setter methods (such as IsAckedWithSecEnhAck, GetAckFrameCounter, GetAckKeyIndex, and GetTxDelay) in radio_frame.hpp and updates their usages across several files. It also removes conditional compilation guards around TX delay methods. There are no review comments, so I have no feedback to provide.

*
* @returns The secured Enh-ACK key index.
*/
uint8_t GetAckKeyIndex(void) const { return AsFrame().mInfo.mRxInfo.mAckKeyId; }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we consider renaming mAckKeyId? It is part of the API, but it is a simple rename.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That is possible, but it would break backward compatibility and cause build failures for all radio vendors (which use this) requiring platform provider to update their implementation. I'm generally a bit hesitant to introduce breaking changes unless we have a strong justification.

In any case, a radio API change like that should probably be handled in its own PR—where we can detail the motivation and rationale—and probably alongside some corresponding updates in the various platform (radio repositories).

@jwhui
jwhui merged commit 06dc96c into openthread:main Jul 20, 2026
98 of 99 checks passed
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.

2 participants