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

Overload begin, abstract some docs, & patch printf for mbed #750

Merged
merged 62 commits into from
Apr 11, 2021

Conversation

2bndy5
Copy link
Member

@2bndy5 2bndy5 commented Mar 20, 2021

  • repo now has a docs folder that contains all pages previously defined in RF24.h
    • this makes maintenance easier than scrolling through 2000+ lines of code
    • these pages are meant for doxygen (using @note and @warning admonitions that only work in doxygen), but they can also be viewed (and hyperlinked to specific @sections) through GitHub
    • the docs mainpage is also abstracted
    • adjusted gitignore to ignore only "html" & "xml" subdirectories in the docs folder. updated Doxygen action accordingly. Doxygen runs fine locally.
  • added fix for mbed platform about printf.h from @TonioChingon in Incompatibility with Arduino NANO 33 BLE. #739
  • added fix for printDetails() do not work on SAMD platform (Adafruit Feather M0) #414 to RF24_config.h concerning ARDUINO_ARCH_SAMD about internally using printf(). This fix is only appicable to the adafruit fork of the ArduinoCore-samd. Original ArduinoCore-samd does not use avr/pgmspace.h and doesn't declare printf() as accessible method from the Serial object(s).
  • Removed useless /utility/Due/R24_arch_config.h file and fixed missing avr/pgmspace.h for Arduino Due (pgmspace was added back into the Due core in 2013).
  • added Arduino Due to the ArduinoCLI workflow. Also introduced a new workflow that uses PlatformIO to test the examples on the Teensy platform (thus the new badge in the README).
  • I also took the liberty of bumping the library version to v1.4.0 (PlatforIO & Arduino) as this includes a new feature.
  • amended isValid() to check if private members ce_pin & csn_pin are 0xFFFF because it seems it wasn't updated when the c'tor parameters changed their datatype from uint8_t to uint16_t

Overloaded begin()

  • new overloaded begin(_SPI*) function for Arduino platform (but not Due, ATTiny, LittleWire, or SPI_UART) that allows the user to specify a non-default SPI bus object
    1. docs/arduino.md has example snippets for
      • NodeMCU (using pins' on-board labels via macros from the ESP8266 arduino core)
      • ESP32 (generic)
      • Teensy (generic) - though I don't think teensy users need to use the overloaded begin(_SPI*) (as demonstrated in example snippet)
      • ATSAMD21 (generic) - example is written but commented out until verified (won't show in official docs)
      • MBED core has predefined SPI and SPI1, and the example using SPI1 is written but commented out until verified (won't show in official docs)
    2. The SPIClass::begin() function must be called before calling RF24::begin() as warned in arduino.md page and the RF24::begin(_SPI*) function's docs (which also defers to the Arduino support page).
    3. this introduces a couple new macros:
      • DOXYGEN_FORCED which is only defined when doxygen is executed. I needed this (implemented in the Doxyfile) so that doxygen would actually show the new docs for the overloaded begin(_SPI*).
      • RF24_SPI_PTR which is only defined when _SPI is used as a datatype (as opposed to the actual SPI object for a platform). This made my head stop hurting when making necessary changes to function calls about the SPI object.
  • new overloaded begin(_cepin, _cspin) to implement feature request from Default constructor request #539. This applies to all supported platforms. begin() returns false if CE & CSN pins are never specified (either with c'tor nor new overloaded begin() methods). This implementation also provides an overloaded c'tor that accepts only the _spi_speed parameter (that still defaults to RF24_SPI_SPEED - so it could essentially be used as an empty c'tor). This feature is extended to the python wrapper.
  • new additionally overloaded begin(_SPI*, _cepin, _cspin) on Arduino platforms to allow also specifying a SPI bus from feature request [Request] allow non-default SPI bus by overloading begin() #743

@2bndy5 2bndy5 marked this pull request as ready for review March 31, 2021 19:48
@2bndy5
Copy link
Member Author

2bndy5 commented Mar 31, 2021

py wrapper now works using empty c'tor + begin(pin, pin) 🎉

I also tested that the old traditional way still works in the py wrapper 👍🏼

I'm good to go for review here. Please pay special attention to the begin() methods. I had to abstract the original algorithm from begin() into _init_pins() and _init_radio()...

@2bndy5 2bndy5 requested review from TMRh20 and Avamander March 31, 2021 20:11
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.

I haven't been able to test everything, but everything I've tested tests OK.

@2bndy5
Copy link
Member Author

2bndy5 commented Apr 9, 2021

I tested esp8266 and the python wrapper; both work as expected. I didn't re-test all the examples; I only tested the gettingStarted.ino against its Linux equivalent - no problems.

I have one concern before merging.

Am I ok to do a "merge commit" instead of "squash and merge"?

The rp2xxx branch (for Pico SDK support) has merged all the changes from this overload-begin branch. If I squash the commits like we usually do, then It would mess with the history on the rp2xxx branch. My usual remedy is to start a new branch and copy-n-paste all files from the messed up branch, but that would/might overwrite proper history/contributions made by @kripton (he deserves much of the credit).

However, if I do a merge commit, then it would only add the 62 commits from this branch to master's history, and the history on the rp2xxx branch should be preserved; there might be a conflict (when we're ready to PR rp2xxx branch) but nothing complicated. I'm open to alternatives (maybe there's some git "magic" I'm not aware of), but its imperative that @kripton gets his due credit.

@kripton
Copy link

kripton commented Apr 10, 2021

I have one concern before merging.

Am I ok to do a "merge commit" instead of "squash and merge"?

The rp2xxx branch (for Pico SDK support) has merged all the changes from this overload-begin branch. If I squash the commits like we usually do, then It would mess with the history on the rp2xxx branch. My usual remedy is to start a new branch and copy-n-paste all files from the messed up branch, but that would/might overwrite proper history/contributions made by @kripton (she deserves much of the credit).

However, if I do a merge commit, then it would only add the 62 commits from this branch to master's history, and the history on the rp2xxx branch should be preserved; there might be a conflict (when we're ready to PR rp2xxx branch) but nothing complicated. I'm open to alternatives (maybe there's some git "magic" I'm not aware of), but its imperative that @kripton gets her due credit.

I assume what could be done is to squash-merge this PR and then rebase the rp2xxx-branch on top of it. That way, the history of the RP2040-port would be kept intact. At least as long as this is then also squash-merged ;)

Side note: I'm a guy :) And honestly, no offence taken, that gender-confusion happens from time to time. My name is more like the greek Yannis than the the American Janis.

@2bndy5
Copy link
Member Author

2bndy5 commented Apr 10, 2021

Side note: I'm a guy :)

So Sorry about that. I should've just resorted to a "he/she" as in "he or she", but thank you for the clarification.

In my past experiences rebasing on a squashed PR led to numerous conflicts, but that's probably from using gitKraken and Github for desktop apps on Windows (as well as the context). I'm still concerned with rebase option because some of the commits on the rp2xxx branch predate some of these commits (clearly I'm no expert).

@kripton
Copy link

kripton commented Apr 10, 2021

Side note: I'm a guy :)

So Sorry about that. I should've just resorted to a "he/she" as in "he or she", but thank you for the clarification.

Don't be sorry, I'm perfectly fine with being a guy 🤣 No, really, you couldn't have known, so no worries.

In my past experiences rebasing on a squashed PR led to numerous conflicts, but that's probably from using gitKraken and Github for desktop apps on Windows (as well as the context). I'm still concerned with rebase option because some of the commits on the rp2xxx branch predate some of these commits (clearly I'm no expert).

Indeed, I just tried that locally and it's not as easy as I'd thought... I guess you would need to decide for every commit to which branch / squash it should be belong. It is possible using the interactive rebase feature of git but since it's getting late here, I won't do that exercise today :)

@2bndy5
Copy link
Member Author

2bndy5 commented Apr 10, 2021

I guess you would need to decide for every commit to which branch / squash it should be belong. It is possible using the interactive rebase feature of git

eesh. I got into this coding stuff to be as lazy as possible (sound american enough for ya? 😆 ).

@Avamander if there are no objections, I'll just do a "merge commit" (no squashing/fast-forwarding)

@Avamander
Copy link
Member

@2bndy5

Avamander if there are no objections, I'll just do a "merge commit" (no squashing/fast-forwarding)

The commits look good and descriptive (although I'd capitalize the first letter 🙂), I think it's quite OK.

@Avamander
Copy link
Member

Avamander commented Apr 10, 2021

Oh, hm, is there a reason "Rebase & merge" isn't ok?

@2bndy5
Copy link
Member Author

2bndy5 commented Apr 10, 2021

is there a reason "Rebase & merge" isn't ok?

image
It seems that's my only option left. I have no problem with that option.

There's already going to be conflicts in the hypothetical PR for rp2xxx branch, but its the history I'm trying to preserve (deferring to nRF24/RF24Log#3 as exemplified experience).

@2bndy5 2bndy5 merged commit 0b60c0b into master Apr 11, 2021
@2bndy5 2bndy5 deleted the overload-begin branch April 11, 2021 00:42
@2bndy5
Copy link
Member Author

2bndy5 commented Apr 12, 2021

@TMRh20 Following a "release often" paradigm, I think it would be better to release the current master branch instead of waiting for the CMake solution (& Pico SDK support). I have already bumped the version number to v1.4.0, and, as usual, I'm willing to amend the release description (heads-up: I'll probably just copy most of this PR's description). This way we can isolate any problems that might get reported for the CMake implementation.

Reminder: the PlatformIO lib archives don't list v1.3.12 due to a missing comma (#742) which is also a fix that @makerMcl is patiently waiting for. 😉

Wait, @Avamander can I publish releases all by myself? I see the option to do so under my current privileges.

@Avamander
Copy link
Member

Avamander commented Apr 13, 2021

Wait, Avamander can I publish releases all by myself? I see the option to do so under my current privileges.

You probably can with commit access, but if you have something you want a second pair of eyes on let me know.

@2bndy5
Copy link
Member Author

2bndy5 commented Apr 13, 2021

Publishing a release feels like a bit more responsibility than a mere contributor should have (because of the Arduino Library Manager integration). I'll continue asking for an "ok" before publishing a release.

That being said, @TMRh20 can I do a release crusade? Actually the RF24Network, RF24Mesh, RF24Ethernet, & RF24Gateway version numbers need a bump before a release crusade should take place. Remember, once the current RF24 master branch is published, some of the other RF24* doc links to this repo's docs will be broken (which is why I recently submitted a PR to all other RF24* libs) until their latest master branches are released.

@TMRh20
Copy link
Member

TMRh20 commented Apr 13, 2021

@2bndy5 Yup, you can add your release notes directly :p

@2bndy5
Copy link
Member Author

2bndy5 commented Apr 13, 2021

done. It seems that RF24 repo is the only repo I can't directly commit to master (for small stuff like a version bump)

@2bndy5
Copy link
Member Author

2bndy5 commented Apr 13, 2021

@TMRh20 @Avamander I've been thinking: If I'm allowed to be an owner, I can bring my CircuitPython_nRF24L01 lib into the nRF24 org. This thought occurred to me when I recently realized that I've somehow modified almost every file in this org's repos. Just throwin that out there (its not like I'm going anywhere - that's not supposed to be a threat 😆 ).

I've been working on porting RF24Network to CircuitPython (needs polish & queue implementation) while also extending the lib to support MicroPython and the 3rd-party SpiDev C-extention module in Linux's CPython.

@TMRh20
Copy link
Member

TMRh20 commented Apr 13, 2021

@2bndy5 I'm good with that, it'll help to bring everything together in one place.

@2bndy5
Copy link
Member Author

2bndy5 commented Apr 13, 2021

I was on the fence about mentioning it because it kinda feels like a bribe for owner privileges. That said, I won't be transferring ownership of my CircuitPython lib until I get an invite to upgrade my nRF24 org privileges; its my first distributed lib ever, so there's some sentimentality there. Its not meant to be a bribe, but having all nRF24L01 libs in 1 place is exactly why I did mention it.

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