Skip to content

add: expose status byte in public API #1032

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

Merged
merged 15 commits into from
May 3, 2025
Merged

add: expose status byte in public API #1032

merged 15 commits into from
May 3, 2025

Conversation

2bndy5
Copy link
Member

@2bndy5 2bndy5 commented Apr 28, 2025

Offers intuitive alternatives to maskIRQ() and whatHappened().
The new API is more explicit (see example modifications), but there's less bit-banging (in lib internal definitions) and memory allocation involved .
The new API is implemented in the python wrapper also.
All new API is grouped in the docs under the topic "Status flags".

Alternative uses

Additionally, this exposes more than just the IRQ flags. Aside from the uses shown in this patch's example modifications, one can also...

uint8_t pipe;
if (radio.available(&pipe)) {
    printf("RX pipe used: %d\n", pipe);
}

can also be done like so:

if (radio.available()) {
    uint8_t flags = radio.update(); // Uses SPI non-op cmd
    printf("RX pipe used: %d\n", (flags >> 1) & 7);
}

and

if (radio.isFifo(true) == RF24_FIFO_FULL)

can also be done (with less internal bit-banging) like so:

StatusFlags flags = radio.update(); // Uses SPI non-op cmd
if (flags & 1)

New and old API

  • setStatusFlags() deprecates maskIRQ()

  • clearStatusFlags() deprecates whatHappened() and allows for clearing all or individual flags.

  • the private method print_status() is now publicly exposed as printStatus()

  • the private method get_status() is now publicly exposed as update()

  • new getStatusFlags() allows read access to the privately cached status attribute. This means NO SPI transactions; that's what update() does now.

  • new rf24_irq_flags_e defines constants to be used as masks.

    • RF24_RX_DR
    • RF24_TX_DS
    • RF24_TX_DF
    • RF24_IRQ_ALL (alias of above or'd together)
    • RF24_IRQ_NONE (alias of 0 to avoid using "magic" numbers)

    These double as values passed to setStatusFlags() and clearStatusFlags(). As with any masks, these constants can be or'd together to specify more than 1 STATUS flag when used with the aforementioned functions.

@2bndy5

This comment was marked as resolved.

@2bndy5 2bndy5 force-pushed the expose-status-byte branch from 6745f03 to 3e5a7ec Compare April 28, 2025 07:59
Copy link
Contributor

github-actions bot commented Apr 28, 2025

Memory usage change @ 3db7cfa

Board flash % RAM for global variables %
arduino:avr:nano ❔ -44 - +2 -0.14 - +0.01 0 - 0 0.0 - 0.0
arduino:samd:mkrzero ❔ -36 - +20 -0.01 - +0.01 0 - 0 0.0 - 0.0
Click for full report table
Board examples/GettingStarted
flash
% examples/GettingStarted
RAM for global variables
% examples/AcknowledgementPayloads
flash
% examples/AcknowledgementPayloads
RAM for global variables
% examples/ManualAcknowledgements
flash
% examples/ManualAcknowledgements
RAM for global variables
% examples/StreamingData
flash
% examples/StreamingData
RAM for global variables
% examples/MulticeiverDemo
flash
% examples/MulticeiverDemo
RAM for global variables
% examples/InterruptConfigure
flash
% examples/InterruptConfigure
RAM for global variables
% examples/scanner
flash
% examples/scanner
RAM for global variables
% examples/encodeRadioDetails
flash
% examples/encodeRadioDetails
RAM for global variables
%
arduino:avr:nano 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 2 0.01 0 0.0 0 0.0 0 0.0 -44 -0.14 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0
arduino:samd:mkrzero 0 0.0 0 0.0 0 0.0 0 0.0 4 0.0 0 0.0 20 0.01 0 0.0 0 0.0 0 0.0 -36 -0.01 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0
Click for full report CSV
Board,examples/GettingStarted<br>flash,%,examples/GettingStarted<br>RAM for global variables,%,examples/AcknowledgementPayloads<br>flash,%,examples/AcknowledgementPayloads<br>RAM for global variables,%,examples/ManualAcknowledgements<br>flash,%,examples/ManualAcknowledgements<br>RAM for global variables,%,examples/StreamingData<br>flash,%,examples/StreamingData<br>RAM for global variables,%,examples/MulticeiverDemo<br>flash,%,examples/MulticeiverDemo<br>RAM for global variables,%,examples/InterruptConfigure<br>flash,%,examples/InterruptConfigure<br>RAM for global variables,%,examples/scanner<br>flash,%,examples/scanner<br>RAM for global variables,%,examples/encodeRadioDetails<br>flash,%,examples/encodeRadioDetails<br>RAM for global variables,%
arduino:avr:nano,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,2,0.01,0,0.0,0,0.0,0,0.0,-44,-0.14,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0
arduino:samd:mkrzero,0,0.0,0,0.0,0,0.0,0,0.0,4,0.0,0,0.0,20,0.01,0,0.0,0,0.0,0,0.0,-36,-0.01,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0

@2bndy5

This comment was marked as resolved.

@2bndy5 2bndy5 force-pushed the expose-status-byte branch 10 times, most recently from e62f3bc to 039c3ce Compare April 29, 2025 02:00
@2bndy5
Copy link
Member Author

2bndy5 commented Apr 29, 2025

Ok, I reverted the use of a 69-byte string buffer (to translate the status byte like print_status() using StatusFlags::toString()) in the irqConfig example, and the flash used went from +3% to -0.18%. 🎉

This proves that the new setStatusFlags() (new maskIRQ()) and clearStatusFlags() (new more versatile whatHappened()) are cheaper because of the new rf24_irq_flags_e enumerated constants (which are just aliases to _BV(flag-mneumonic)).

Now I'm curious if replacing other internal uses of the status byte will be optimized as well. 🤔

I'm going to cleanup the git history and keep at it. I haven't been hardware testing this yet. I just wanted to ensure compile size wasn't impacted negatively.

2bndy5 added 2 commits April 28, 2025 19:28
Offers intuitive alternatives to `maskIRQ()` and `whatHappened()`.
The API  is more explicit, but there's less bit-banging and memory allocation involved.
The StatusFlags API is implemented in the python wrapper also.
All new API is grouped in the docs under the topic "Status flags".
@2bndy5 2bndy5 force-pushed the expose-status-byte branch 2 times, most recently from d13ae51 to 756e6a9 Compare April 29, 2025 04:30
@TMRh20
Copy link
Member

TMRh20 commented Apr 29, 2025

This is looking good, but one critique, would be that I'm not sure we want to deprecate the whatHappened() and maskIRQ functions, because I think we should leave them in the examples.

To you it may seem simple, but I think this represents advanced usage, whereas a simpler user approach is to use the current API, which is what should be represented in the examples.

Maybe converting everything and deprecation can be left for v2.0?

@2bndy5
Copy link
Member Author

2bndy5 commented Apr 29, 2025

TBH, I always hated1 the maskIRQ() and whatHappend() functions.

  1. The arguments to maskIRQ() literally require me to look up the docs every time I use it. Furthermore, the boolean values are flipped compared to human understanding. 1 means disable event, 0 means enable event. And I often forget/confuse what order they're supposed be in.
  2. whatHappened() has required arguments. If the user code only cares about RX_DR flag, they need to allocate variables for TX_D* values as well. And yet again with the proper order of args here too.

Clang-tidy actually has a lint rule to flag these functions as "prone to bugs".

Footnotes

  1. "hate" not an exaggeration here

@2bndy5
Copy link
Member Author

2bndy5 commented Apr 29, 2025

Maybe converting everything and deprecation can be left for v2.0?

Deprecated API should be deprecated for a long enough time before removal in the next major version (v2). Since we have no plans to actually pursue v2.0, Users should have ample time to update the lib and migrate to new API. It is considered breakage if a deprecated API is removed in a minor or patch release.

Tip

The new Arduino IDE will cross-out deprecated API in the editor thanks to the @deprecated doc comment.

@2bndy5
Copy link
Member Author

2bndy5 commented Apr 29, 2025

To be fair, providing constants to be used as masks is a common approach to exposing a bit-field. The Pico SDK has used this approach for many things rather successfully.

@2bndy5
Copy link
Member Author

2bndy5 commented Apr 29, 2025

After converting the internal API to use the new API, I only saw a 4 byte increase in flash size on ATSAMD21 for the ACK examples. I'm looking into that now.

The IRQ example's flash size actually went down on both AVR and ATSAMD21.
The StreamingData example went up slightly (negligible) due to the additional flags variable and if block in the TX role.

2bndy5 added 3 commits April 29, 2025 00:02
don't use `printStatus()` in arduino IRQ example
@2bndy5 2bndy5 force-pushed the expose-status-byte branch from de4bb60 to dbb4d00 Compare April 29, 2025 07:05
@TMRh20
Copy link
Member

TMRh20 commented Apr 29, 2025

Hehe, with your unrefined hate for those functions, I guess I'll withdraw my previous comments.

@2bndy5 2bndy5 marked this pull request as ready for review April 29, 2025 19:22
@2bndy5
Copy link
Member Author

2bndy5 commented Apr 29, 2025

I'm going to start a migration guide to highlight the differences in API for v1.5.

@2bndy5
Copy link
Member Author

2bndy5 commented May 1, 2025

My hardware tests on Linux (with and with python wrapper) now show this working successfully. 🎉

I still need to test the Arduino and Pico SDK examples...

@2bndy5 2bndy5 requested a review from TMRh20 May 3, 2025 00:04
Copy link
Member

@TMRh20 TMRh20 left a comment

Choose a reason for hiding this comment

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

Quick tested on Arduino with interruptConfigure & streaming examples

Copy link
Member Author

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

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

final review

@2bndy5 2bndy5 merged commit 0bd92d5 into master May 3, 2025
196 of 197 checks passed
@2bndy5 2bndy5 deleted the expose-status-byte branch May 3, 2025 06:36
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