nRF24 / RF24

OSI Layer 2 driver for nRF24L01 on Arduino & Raspberry Pi/Linux Devices
https://nrf24.github.io/RF24
GNU General Public License v2.0
2.23k stars 1.02k forks source link

better auto-ack representation in printPrettyDetail() #706

Closed 2bndy5 closed 3 years ago

2bndy5 commented 3 years ago

Currently, the representation of auto-ack feature in the new printPrettyDetails() is dependent on the feature's configuration about pipe 0. I'll admit, this was just laziness on my behalf, but @combs meticulous testing has made a case for addressing this mistake.

So, I've been thinking... What if we use the following snippet to represent auto-ack feature:

    uint8_t autoAck = read_register(EN_AA);
    if (autoAck == 0x3F || autoAck == 0) {
        // all pipes have the same configuration about auto-ack feature
        printf_P(PSTR("Auto Acknowledgment\t"
        PRIPSTR
        "\r\n"), (char*)pgm_read_ptr(&rf24_feature_e_str_P[(bool)(autoAck) * 1]));
    } else {
        // representation per pipe
        printf_P(PSTR("Auto Acknowledgment\t= 0b%c%c%c%c%c%c\r\n"),
                 (char)((bool)(autoAck & _BV(ENAA_P5)) + 48),
                 (char)((bool)(autoAck & _BV(ENAA_P4)) + 48),
                 (char)((bool)(autoAck & _BV(ENAA_P3)) + 48),
                 (char)((bool)(autoAck & _BV(ENAA_P2)) + 48),
                 (char)((bool)(autoAck & _BV(ENAA_P1)) + 48),
                 (char)((bool)(autoAck & _BV(ENAA_P0)) + 48));
    }

The problem I ran into during development was that %s replaced with a "0bxxxxxx" wasn't outputting correctly (seemed like the pointer to the string was not de-referencing properly). But, the use of %c worked fine for the "Primary Mode" output... For those that noticed what @combs noticed, we are aware of the wrong mnemonic used for the "primary mode" representation. Its a simple fix that should be included with the PR that addresses this issue.

2bndy5 commented 3 years ago

The above proposal works. I tested the changes using the python wrapper because it was easier than writing/modifying a Linux C++ example. The following results only show relevant info from output for brevity/concisness.

>>> radio.setAutoAck(0)
>>> radio.setAutoAck(1, 1)
>>> radio.setAutoAck(3, 1)
>>> radio.printPrettyDetails()
Auto Acknowledgment     = 0b001010

I also fixed the "primary mode" representation using the PRIM_RX mnemonic.

>>> radio.startListening()
>>> radio.printPrettyDetails()
Primary Mode            = RX
>>> radio.stopListening()
>>> radio.printPrettyDetails()
Primary Mode            = TX

@combs If you can think of a printf() friendly way to only output the pipes that have auto-ack enabled, I'll be glad to test it for you. Otherwise I'm going forward with a PR to resolve this.

2bndy5 commented 3 years ago

This proposal costs an additional 138 bytes

without printPrettyDetails() on arduino nano

Sketch uses 6548 bytes (21%) of program storage space. Maximum is 30720 bytes. Global variables use 247 bytes (12%) of dynamic memory, leaving 1801 bytes for local variables. Maximum is 2048 bytes.

with printPrettyDetails() on arduino nano

Sketch uses 10966 bytes (35%) of program storage space. Maximum is 30720 bytes. Global variables use 263 bytes (12%) of dynamic memory, leaving 1785 bytes for local variables. Maximum is 2048 bytes.

with printPrettyDetails() on arduino nano BEFORE changes

Sketch uses 10828 bytes (35%) of program storage space. Maximum is 30720 bytes. Global variables use 263 bytes (12%) of dynamic memory, leaving 1785 bytes for local variables. Maximum is 2048 bytes.

Avamander commented 3 years ago

I assume people will remove printPrettyDetails() when they are finalizing their project so the space usage is totally fine.

2bndy5 commented 3 years ago

@Avamander good point @combs I think the binary representation is a little less cryptic than only outputting the pipe numbers that have auto-ack enabled. I'm going forward with this solution.