Skip to content
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

remove useless ce() call #987

Merged
merged 1 commit into from
Sep 9, 2024
Merged

remove useless ce() call #987

merged 1 commit into from
Sep 9, 2024

Conversation

2bndy5
Copy link
Member

@2bndy5 2bndy5 commented Jul 15, 2024

resolves #986

Copy link
Contributor

github-actions bot commented Jul 15, 2024

Memory usage change @ e5ed6c0

Board flash % RAM for global variables %
arduino:avr:nano 💚 -6 - 0 -0.02 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:mkrzero 💚 -8 - 0 -0.0 - 0.0 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 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 -6 -0.02 0 0.0 0 0.0 0 0.0
arduino:samd:mkrzero 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 -8 -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,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,-6,-0.02,0,0.0,0,0.0,0,0.0
arduino:samd:mkrzero,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,-8,-0.0,0,0.0,0,0.0,0,0.0

@2bndy5
Copy link
Member Author

2bndy5 commented Aug 2, 2024

Testing this would require running the examples/scanner/scanner.ino in Arduino IDE (using a radio that returns true for isPVariant()) and any code that calls reUseTX() or writeBlocking() with an unreliable connection to the other node. The only example that uses writeBlocking() is

if (!radio.writeBlocking(&data, 32, timeoutPeriod)) { // If retries are failing and the user defined timeout is exceeded

However, I haven't focused on maintaining those old examples...

I think its worth noting that the CirPy lib has been using this patch for years, and I haven't had any problems reported there.

@2bndy5
Copy link
Member Author

2bndy5 commented Aug 2, 2024

Oh, the streamingData examples use reUseTX()! I tested the example (on Linux and PicoSDK) but had no failures reported, meaning no call to reUseTX().

The carrier wave is properly transmitting from examples/scanner/scanner.ino with a radio that returns true for isPVariant(). I'll take that as a good sign.

@2bndy5
Copy link
Member Author

2bndy5 commented Aug 11, 2024

I think its worth noting that the CirPy lib has been using this patch for years, and I haven't had any problems reported there.

Actually, the CirPy lib just clears the status flags and relies on the fact that the radio will attempt to transmit anything in the TX FIFO once the CE pin is cycled. The CirPy lib does not actually use the REUSE_TX_PL command.

    if self.fifo(about_tx=True, check_empty=True):
        return False  # return early if nothing in TX FIFO
    self._ce_pin.value = False
    self.clear_status_flags()  # clear all IRQ flags
    # self._reg_write(0xE3)  # skips REUSE_TX_PL command because TX FIFO is occupied
    self._ce_pin.value = True

I think I did this to allow the radio to move onto the next payload when successful. Otherwise, the radio is stuck re-transmitting the same payload in the top level of TX FIFO until any of the following conditions:

  • TX failure
  • TX FIFO flushed
  • write new payload to TX FIFO (excluding ACK payload uploads). This condition does not guarantee that the new payload gets put in top level of TX FIFO. Furthermore, it is unclear if the re-used payload is discarded once a new payload deactivates the REUSE_TX_PL command. This may cause one duplicated transmission when processing the TX FIFO up to the new uploaded payload, especially if the REUSE_TX_PL is issued when TX FIFO has more than 1 payload queued.

According to datasheet (end of section 7.5.2):

As an alternative to Auto Retransmit it is possible to manually set the nRF24L01+ to retransmit a packet a number of times. This is done by the REUSE_TX_PL command. The MCU must initiate each transmission of the packet with a pulse on the CE pin when this command is used.

@2bndy5
Copy link
Member Author

2bndy5 commented Sep 9, 2024

I have confirmed that this still works. I used the python wrapper to manually create a failed transmission (using writeFast() when no other radio was listening), then invoke reUseTX() when there was another radio ready to receive.

I should still point out that reUseTX() basically spams the same payload until

  • the transmission fails
  • the TX FIFO is flushed
  • a new payload is written
  • setting the CE pin LOW. This lib does not directly expose controlling the CE pin, so invoking reUseTX() from user-code on Linux seems unproductive (and slightly counter-intuitive).

Any objections to merging this?

@TMRh20
Copy link
Member

TMRh20 commented Sep 9, 2024

Nope, I haven't had a chance to test this, but looks like you have! Looks good!

@2bndy5 2bndy5 merged commit 5e89cc8 into master Sep 9, 2024
76 checks passed
@2bndy5 2bndy5 deleted the rm-useless-ce()-call branch September 9, 2024 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CE set LOW twice consecutively in startConstantCarrier()
2 participants