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

Community appreciation #735

Merged
merged 4 commits into from
Feb 26, 2021
Merged

Community appreciation #735

merged 4 commits into from
Feb 26, 2021

Conversation

2bndy5
Copy link
Member

@2bndy5 2bndy5 commented Feb 26, 2021

This PR fixes a couple changes I made in #691 that seemed to upset community members concerning backward compatibility.

  1. open pipes 0 & 1 in begin()noticed in write(...) returns false on sucess unless stopListening() is called first. #734
  2. enableAckPayloads() also enables dynamic payloads for pipe 1 automatically. Reddit and Arduino forum users have gotten quite upset about this one (to which my reaction has been "RTFD" or "pay more attention to examples' comments").

Concerning WiringPi build

Also, I'm disabling the WiringPi CI build to discontinue "Linux build CI failed" notifications that I keep getting. I've successfully built WiringPi locally on my RPi2 (and subsequently RF24 built successfully) using upstream WiringPi repo... I think that libcrypt.so.1 is not available for cross-compilation on x86 Ubuntu, but it seems to ship with armhf-based OSs (like Raspberry Pi OS). Pretty sure that libcrypt is part of the libc6 std libs, but I may be wrong. Lastly, I've changed the Linux build workflow to use the upstream WiringPi fork per @CoRfr recommendation (but it is disabled anyway).

@2bndy5 2bndy5 requested a review from TMRh20 February 26, 2021 04:05
@Avamander
Copy link
Member

Avamander commented Feb 26, 2021

@2bndy5

a) I wouldn't revert 1. and 2. if it can cause hassle for those that have already updated their sketches. We should've maybe said better that behavior changes, but sketches should contain the version of library they were tested with.

b) stopListening being required before write would maybe be under write's docs? I will update the release notes to contain info about the change as well.

c) Is there a way enableAckPayload could make sure the ACK feature is enabled and usable without wasting a lot of performance?

@2bndy5
Copy link
Member Author

2bndy5 commented Feb 26, 2021

Both points shouldn't cause any hassle with either old or new sketches.

Point 1 actually aligns with what the datasheet says about PoR condition (still recommend calling stopListening()...).

Point 2 would cure problems that some people are experiencing with sketches prior to v1.3.11; namely those that aren't aware the docs and examples indicate ACK payloads require dynamic payloads feature.

Is there a way enableAckPayload could make sure the ACK feature is enabled and usable without wasting a lot of performance?

The true problem is that using dynamic payloads (pre-req for ACK payloads) requires configuring 2 registers (DYNPD & FEATURE -- unfortunately the nRF24 slave device doesn't support configuring sequential registers with 1 SPI transaction), so I don't think performance can be improved. Additionally, when dynamic payloads are enabled, RX nodes without dynamic payloads enabled cannot receive payloads from TX nodes with dynamic payloads enabled, so it's probably best to not call enableDynamicPayloads() from within enableAckPayload(). TMRh20 and I brushed upon this topic when he disputed my notion to make dynamic payloads enabled by default, and the notion seemed to have consequences on RF24Network due to supporting counterfeits/clones.

stopListening being required before write would maybe be under write's docs?

Yes! Can't believe I didn't add that. Is that what you call "footgun"? Maybe also add a comment to examples about this? This requirement is nothing new, so adjusting any release notes seems redundant to me. Actually, there is a code snippet in write()'s docs that demonstrates proper usage of stopListening(), but maybe that's not enough. It's more likely that people don't know how to navigate the docs, so they don't bother reading them (yet another reason to update the doxygen docs to a more modern "theme" -- I have made progress on that).

@2bndy5 2bndy5 merged commit 57ab538 into master Feb 26, 2021
@2bndy5
Copy link
Member Author

2bndy5 commented Feb 26, 2021

@TMRh20 I think a new release is in order. I can retroactively adjust the release description with my current privileges if that makes it easier for you. For my next trick, I want to overload begin() (for Arduino platform only) to take a reference to a non-default SPI object, but an addition like that would be better categorized as v1.4.0 because it's a new feature (not just a patch/hotfix).

EDIT: Please see #737 first

@2bndy5 2bndy5 deleted the community-appreciation branch February 26, 2021 21:48
@TMRh20
Copy link
Member

TMRh20 commented Feb 27, 2021

I think a new release is in order. I can retroactively adjust the release description...

Sure, I added info about my change, so if you would add info for yours that would be great!

For my next trick, I want to overload begin()

Cool, will wait to see how it comes out

@2bndy5
Copy link
Member Author

2bndy5 commented Feb 28, 2021

if you would add info for yours that would be great!

done. I get a notification about releases also, so I will continue to add info about my submitted changes in the future (if necessary).

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.

3 participants