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

new examples and more #691

Merged
merged 198 commits into from
Dec 14, 2020
Merged

new examples and more #691

merged 198 commits into from
Dec 14, 2020

Conversation

2bndy5
Copy link
Member

@2bndy5 2bndy5 commented Nov 29, 2020

This is the PR I've been teasing about. All doc changes are rendered for review at https://2bndy5.github.io/RF24 -- some links that lead to nRF24.github.io/RF24 are dead until this PR gets merged.

Issues Addressed

issue title notable changes
Simplify Examples #658 See this comment for what was removed
Doxygen automated upload to tmrh20.guthub.io #668 links in tmrh20.github.io repo's index.html file need alteration for directing to docs hosted by individual repos
need to merge my PR on tmrh20.github.io repo
new printPrettyDetails() for a more
human readable printDetails() #687
printPrettyDetails() currently takes about 580 more bytes than printDetails()
Move REGISTER_MASK to print_byte_register() &
replace spiTrans() with write_register() #686
only the get_status() transaction doesn't print to Serial when SERIAL_DEBUG is defined
writeBlocking(), writeFast(), & txStandBy() functions could be slightly faster #678 Essentially this saves the STATUS byte from MISO on every
SPI transaction to the new private member status
pinMode(IRQ_PIN, INPUT) needed in examples #680 basic fixes to old pingpair_irq.ino example
Error setting PALevel in python3 #677 setPALevel() takes 1 or 2 args properly
let startWrite() return a bool #679 returns false if TX FIFO is full or
true if payload was written to TX FIFO
Move configuration of all RX_PW_Px registers to setPayloadSize() #674 allows calling setPayloadSize() independently
from when openReadingPipe() is called
payload length is now clamped to range [1, 32]
Adding a CI workflow for compiling
examples with Arduino CLI #665
only new examples are being compiled to save some time
(takes ~ 5 minutes to complete); rf24_ATTiny examples are compiled with the SpenceKonde ATTinyCore
startListening() calls closePipe(0) #671 only affects docs about using pipe 0 for RX mode
specify SpenceKonde's ATTiny core in rf24_ATTiny examples' docs also adjusted examples' code in rf24_ATTiny folder accordingly
ignoring a return value in interrupt.c file #636 doesn't use proper error handling, but
the solution does get rid of the warning during compilation
setAutoAck() needs better documentation #667 desperately needed for better reference when helping
troubleshoot for newcomers that blindly follow unofficial tutorials
There is an issue about the "toggle_feature()" after mcu reset #401 brute force a soft reset in begin(); uses before-n-after
comparison to set the new _is_p_variant private member
docs on reUseTx() need some clarification #655 helps clarify how reUseTX() works behind the curtain
Deprecate isAckPayloadAvailable() #664 simple docs change (using @deprecate)
use ack_payloads_enabled private member to full potential #654 avoids unnecessary SPI transactions
writeAckPayload() can be called before and after calling startListening()
startCarrierWave() may be incompatible with older nRF24L01 (non-plus variants) #640 improves non-plus variant compatibility in startCarrierWave() using new _is_p_variant private member
setting the data rate may not be the best implementation of isPVariant() #641 uses new _is_p_variant private member
why is flush_rx() marked for deprecation? #651 simply removes flush_rx() from deprecation status in docs
pipe 1 does not need dynamic payloads enabled to transmit ACK payloads #653 enableAckPayload() doesn't force-enable dynamic payloads feature for pipe 1;
docs and examples show proper manipulation of dynamic payloads feature
Are the docs on writeBlocking() & writeFast() erronious? #689 doc updates about returned data
is the python c'tor comment out of date? #688 simple update to comments about the generic non-RPi SoC boards in python examples
CSN Pin = 0 on getting started linux #684 output for printDetails() (& printPrettyDetails()) now prints
"/dev/spidev<bus_number>.<bus_cs_pin>" about the CSN pin
to avoid any further confusion

Other changes not related to an open issue

  • writeAckPayload() returns a bool describing if payload was or wasn't loaded into TX FIFO (courtesy of the new status private member)
  • dynamic payloads written to TX FIFO are no longer truncated according to the static payload length setting (RF24::payload_size), rather they are truncated to 32 bytes if needed.
  • all differences in python wrapper's API vs C++ API are documented with example code snippets now
  • linux workflow now also builds linux examples (except for wiringPi -- see compiling gettingstarted for wiringPi fails #669 for more detail on that)
  • warn the user (in docs and IRQ examples) about calling available() during th IRQ pin FALLING transition (information is "unreliable" per datasheet -- tests when developing interruptConfigure.ino example showed that available() returned false when it should've been true). Calling whatHappened() before available() seems to allow enough time for pipe information to become accurate again.
  • added more generic information to COMMON_ISSUES.md. This file now provides insight to what settings need to match on each endpoint. Also, this file doesn't assume that users know what "ACK" stands for. Doxygen is now auto-linking to RF24::functions' docs where they are mentioned in COMMON_ISSUES.md
  • my VSCode settings have automatically removed trailing whitespace for any file I saved changes to (see RF24_config.h as an example).
  • properly documented the macros used for configuring CRC, PALevel, & DataRate. Also adjusted the coresponding functions' docs to use tables and link to documented macros.
  • custom-doxygen.css file is formatted properly/consistently. Added a couple css rules to fix un-readability in new docs about macros (see previous point).
  • removed all build artifacts (including Jamfiles) from old examples that were uploaded by maniacBug.
  • updated keywords.txt file (seemed to have been overdue).
  • formatted crossunixcompiler.py & all setup.py files for PEP8 compliance
  • added the use of macros for when RF24_TINY is defined to customize the CSN settling times in RF24::csn(). This should expedite the use of output values from timingSearch3pin.ino for NerdRalph's "3-pin solution".

@2bndy5 2bndy5 mentioned this pull request Dec 7, 2020
Copy link
Member

@TMRh20 TMRh20 left a comment

Choose a reason for hiding this comment

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

So many changes!

The only thing I've noticed is the doc changes with the documenting of GPIO and SPI functions for porting. They now show up in the main class area instead of just RF24. I think this change should be reverted and/or done in a way that it doesn't show up in the main class documentation.

@2bndy5
Copy link
Member Author

2bndy5 commented Dec 10, 2020

I figured that's why the lines declaring the classes were commented out. I had hoped that by adding a @brief description that showed up in the docs' class list it would not be a problem. Although I understand how having their appearance in the class list would be like nerdy click bait.

I did look into using the Doxyfile's EXCLUDE_SYMBOLS or doxygen's @private cmd to exclude them from the list, but these options exclude them entirely from the docs... I'll revert the template files to comment out the class definitions.

@2bndy5
Copy link
Member Author

2bndy5 commented Dec 10, 2020

ok the GPIO & SPI classes don't show up in the class list, but their functions still get documented in "Porting: GPIO", "Porting: SPI" module pages. I've updated the docs preview using a new pre-release candidate on my fork.

So many changes!

There's no reason to go easy on me here. I could see how having to review all these changes might be annoying.

TMRh20
TMRh20 previously approved these changes Dec 12, 2020
@2bndy5
Copy link
Member Author

2bndy5 commented Dec 12, 2020

@Avamander Do you have the time to review this?

@2bndy5
Copy link
Member Author

2bndy5 commented Dec 13, 2020

Oops, I didn't know changing the readme link to the docs would dismiss the approving review.

To host the docs using gh-pages we need to tag a commit for release. This is why I'm not too quick to merge this (& why I preemptively changed the doc link in the readme).

  • Again the key ingredient for using gh-pages is to set the root dir of the gh-pages branch to the "source" option in the repo's settings under the "GitHub Pages" section.

I've already submitted/merged changes to all other nRF24 RF24* libraries, but there needs to be a (what I like to call) "release crusade" across all these RF24* repos (except RF24Audio) before we merge my PR for tmrh20.github.io.

@2bndy5 2bndy5 requested a review from TMRh20 December 13, 2020 18:47
@TMRh20
Copy link
Member

TMRh20 commented Dec 14, 2020

I've already submitted/merged changes to all other nRF24 RF24* libraries, but there needs to be a (what I like to call) "release crusade" across all these RF24* repos (except RF24Audio) before we merge my PR for tmrh20.github.io

Release crusade done, I just did a new release for all the RF24* libs.

@2bndy5 2bndy5 merged commit d69f751 into nRF24:master Dec 14, 2020
@2bndy5
Copy link
Member Author

2bndy5 commented Dec 14, 2020

I hate to say this, but I meant this PR was supposed to be merged before the next release (if you wanted this PR's changes included in 1.3.10).

@TMRh20
Copy link
Member

TMRh20 commented Dec 14, 2020 via email

@Avamander
Copy link
Member

Avamander commented Dec 14, 2020

@Avamander Do you have the time to review this?

I took a relatively short look, but I noticed nothing significant. Everything seems good.

TMRh20 pushed a commit that referenced this pull request Dec 18, 2020
* fix outdated install instruction in python wrapper docs

since I rewrote all the linux examples I noticed the install instructions for the python wrapper direct the user to edit/run an example that no longer exists.

This is a quick fix for just that. (changes tested/rendered correctly on my local machine)

* removed atrifacts from gettingStarted.cpp about cmd args

* ackPl.cpp isn't using anything from cstring.h

* remove CLI artifacts from irqConfig.cpp example

* rem CLI artifacts from manAck.cpp example

* change call to getDynPaylSize() to getPayloadSize() in getStarted.ino

* change call to getDynPaylSize() to getPayloadSize() in getStarted.cpp
@combs
Copy link

combs commented Dec 22, 2020

hello! I'm seeing at least one breaking changes in my Python code, maybe it makes sense to call that out a little more?

radio.setChannel(ch) -> radio.channel = ch

(anything else?)

@2bndy5
Copy link
Member Author

2bndy5 commented Dec 23, 2020

Hmm. I forgot I did that. It is documented.

To be honest, it should've been like that from the start. set/getPayloadSize was properly wrapper to an attribute payloadSize. CRC Length & Data Rate could also use this attribute treatment...

I could undo it for consistency. The Python wrapper should have it's own symantic versioning (and soon will when pyRF24 repo gets its legs).

(Anything else?)

I think you found that 1 thing that wasn't mentioned because none of the new python examples ever call radio.channel. Obviously I could be wrong, and you're welcome to try and find more.

@combs
Copy link

combs commented Dec 23, 2020

Cool, thanks! Yeah I like the new syntax, just caught me by surprise when some previously stable code fell over. Migrating to attributes like that makes sense to me as part of a major version. Appreciate all the new work on this library, it's great!

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