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

added disableDynamicPayloads() #318

Merged
merged 2 commits into from
Jan 4, 2017
Merged

added disableDynamicPayloads() #318

merged 2 commits into from
Jan 4, 2017

Conversation

soligen2010
Copy link
Contributor

@soligen2010 soligen2010 commented Dec 17, 2016

Ack payloads are also disabled at the same time because they depend on
dynamic payloads.

This is needed because I have an old protocol where the TX and RX use fixed length, and want the new code using dynamic lengths to be backwards compatible with the old receivers.

Ack payloads are also disabled at the same time becasue they depend on
dynamic payloads.
@soligen2010
Copy link
Contributor Author

Since TMRh20 added me as a collaborator, I can now merge pull requests. Is there any process I should follow here? (seems like there should be a review or something first). My pull request is pretty simple and there is a lot in this library I don't fully understand. I just don't want to step on anything. Any info on the process you guys use for development/publishing/versions/reviews, etc on this library?

@Avamander
Copy link
Member

Does enableDynamicPayloads restore the state as it was before disableDynamicPayloads?

@soligen2010
Copy link
Contributor Author

disableDynamicPayloads turns off both dynamic payloads and ack payloads (because ack payloads needs dynamic payloads to be enabled). enableDynamicPayloads will only turn the dynamic payloads back on. If you were using ack payloads previously and want to re-enable then too, then these would have to be explicitly turned back on in code. This seems like reasonable behavior to me because there is a dependency here, and next time the dynamic payloads are enabled, there is no way to know if the user would want ack payloads re-enabled too or not. Seem right to you?

Copy link
Member

@Avamander Avamander left a comment

Choose a reason for hiding this comment

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

Please make a commit that indents line 1263 as the rest of the code. Then I can squash and merge this, though I think the comment about disabling ACK should also include that it has to be manually re-enabled.

@Avamander Avamander merged commit ab738fd into nRF24:master Jan 4, 2017
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