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

[Request] use ack_payloads_enabled private member to full potential #654

Closed
2bndy5 opened this issue Oct 18, 2020 · 0 comments
Closed

[Request] use ack_payloads_enabled private member to full potential #654

2bndy5 opened this issue Oct 18, 2020 · 0 comments
Assignees

Comments

@2bndy5
Copy link
Member

2bndy5 commented Oct 18, 2020

The RF24 class has an private variable named ack_payloads_enabled that (I assume from the name & current usage) saves the configuration of the EN_ACK_PAY bit of the FEATURE register. This private variable defaults to false and is only adjusted in constructor, enableAckPayload(), and disableDynamicPayloads(). This private variable is only called upon in the startListening() function while stopListening() reads the EN_ACK_PAY flag directly from the FEATURE register.

Requested usage

  1. add a disableAckPayload() that sets ack_payloads_enabled to false so we wouldn't have to check the EN_ACK_PAY flag in the FEATURE register when setAutoAck() manipulates the auto-ack feature about pipe 0. Simply put, I'd rather have setAutoAck() call disableAckPayload() when disabling the auto-ack feature for pipe 0.
  2. don't use ack_payloads_enabled in startListening() or stopListening() to flush the TX FIFO (see below additional context). Use ack_payloads_enabled in stopListening() to flush the TX FIFO (in preparation for normal TX mode).
  3. use if (ack_payloads_enabled) in writeAckPayload() to verify the radio is properly configured to append ACK payloads to the ACK packets. I understand that the radio may not allow the SPI command W_ACK_PAYLOAD if its not enabled by asserting EN_ACK_PAY flag in the FEATURE register, but using if (ack_payloads_enabled) costs us practically nothing to avoid the attempted SPI transaction (which includes a delay time).
  4. Similar to how powerUp() uses the config_reg private member to avoid unnecessary SPI transactions, we should use ack_payloads_enabled in enableAckPayload() & disableAckPayload() to avoid the unnecessary SPI transactions.

Additional context

I'm not sure the intent of this comment from startListening():

Can update stopListening() to use config_reg var and ack_payloads_enabled var instead of SPI rx/tx

I don't understand why its important to flush the TX FIFO when entering RX mode. In my experience ACK payloads can loaded into the TX FIFO despite what the CONFIG register's PRIM_RX flag (radio primary role) is set to. I know the datasheet says "Used in RX mode" in the description for the SPI command W_ACK_PAYLOAD, but (trust me) it works in either primary role (RX or TX). FYI it also says the same/similar note about FLUSH_RX & FLUSH_TX commands, but this library evidently (& successfully) uses them without verifying the "proper primary role" of the radio.

See also #653

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant