add new feature encodeRadioDetails and optimize sprintfPrettyDetails #825
Conversation
fix auto format, reinsert new functions
add bool packing method to encode_bit_manipulation_methods
update function documentation, work on adding default items to bit array
finish adding members to bit array, add partial debugging output to function
add methods needed to get/unpack interesting values from encoded_details
logic errors in decode_bit_manipulation_methods fully decode encoded_details, ready for output logic
update encode order, fix logic errors in decode_bit_manipulation_methods
roughed in output for decodeRadioDetails
output is working, incorrectly but it's working.
bit array is indexing correctly and the correct amount of information is being encoded
getting expected output from decodeRadioDetails, we might be able to refactor some of this into sprintfPrettyDetails by giving it a default argument and switching to unpack if that argument is nonzero
remove debugging output
update example code for doxygen
|
@dstroy0 Please enable all workflows on your fork. Typically this can be done by going to https://github.com/dstroy0/RF24/actions, but I can't confirm this because I don't have admin privileges for your fork (and I do not want admin rights for your fork). Please also check the repo settings for your fork at https://github.com/dstroy0/RF24/settings/actions I just want to be sure that the changes you just submitted after opening the PR fixed the PicoSDK and Linux builds. |
I enabled all of the workflows. I can see them but there's no run workflow button. |
fixed a syntax error in the example code for decodeRadioDetails
|
You can't trigger them manually because they're not setup for that (a rather advanced use case). You should be able to trigger the Linux & PicoSDK workflows by making a commit that edits the lib source code somehow; I usually just add a bogus comment or new line somewhere to trigger them. The Arduino CI and PlatformIO CI workflows are triggered by any commit involving the examples folder (not examples_linux or examples_pico folders). Again, I usually just add a bogus comment or new line somewhere to trigger them. |
|
Judging from the latest runs on your fork, the Linux builds and Pico SDK builds look ok again ✅ |
|
Fun fact: I setup the docs CI to upload built docs (folder of HTML files) as workflow artifacts which you can download and open on your local machine for inspection. |
comment out unused method encode_bit_manipulation_methods::put32BitValueIntoOutputArray()
Excellent, forgetting to remove that debug output before issuing the pr was a big oof. |
create example using encodeRadioDetails(), can be used for testing to verify changes to bit array members before deployment
|
could maybe add the functions to KEYWORD 2 so the arduino ide highlights them in orange |
No objections there. In fact, I didn't check if the same was applied to |
Co-authored-by: Brendan <2bndy5@gmail.com>
|
Ran a local test about my idea for sprintfDets using the new example encodeRadioDets.ino: as is (without multiple calls to sprintf)with multiple calls to sprintf (& no additionally allocated local buffers)Looks like flash goes down by 438 bytes, but ram goes up by 4 bytes. This is an acceptable balance for me. I'll start cleaning up the tested code and commit, so you can play around a bit further (which might be better than me trying to explain what I did). sprintfDets and sprintf_reg_addr now return the number of bytes that were altered in the given buffer (just like sprintf does). sprintf_reg_byte is now removed. EDITED: During clean-up, I saw more reduction in flash 👍🏼 I also found inconsistent indentation in sprintfDets (I think this was overlooked in #821 ). @dstroy0 When you run clang-format, do you have this .clang-format file located in the repo's root? If not then the formatting is following LLVM code style which is different (much stricter) than the rules I've setup for this repo (on the clang-format branch). |
|
Everything looks great! I wont have a chance to test anything until this evening. I will figure out how to make the clang-format extension use that format specifier. |
|
I got clang-format to use the .clang-format file |
factor out duplicate index
4 bytes of ram saved on avr back down to 232 bytes global usage
YES! That's what I was going for: Reduced flash, and don't add to ram. @dstroy0 Thanks for fixing my mistakes. |
2bndy5
changed the title
|
I just had another itch to re-use the lib's private SPI buffers ( |
|
Nope nevermind. I forgot that those private SPI buffers only exist on platforms that have more memory #if defined (RF24_LINUX) || defined (XMEGA_D3) || defined (RF24_RP2)
|
|
Everything looks great to me, I’ve been using encodeRadioDetails and sprintfRadioDetails as part of some of my projects startup status dump and they work as intended. I’ll be sure to open an issue in the future before doing a PR. This was a lot of fun.
… On Jan 29, 2022, at 8:07 PM, Brendan ***@***.***> wrote:
@2bndy5 approved this pull request.
I'm ready to merge this unless you can think of more optimizations
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
Yes. Please do as that would provide a way to have separate discussions about ideas and code review |
encodeRadioDetails will encode an array of 10 uint32_t (40 bytes) with the exact same information that is found in printPrettyDetails.
decodeRadioDetails works in the same manner as sprintfPrettyDetails, but you pass it the encoded array and it formats the output buffer for you
This is useful for anyone needing status information with no output and stringent memory use requirements.
encodeRadioDetails is meant to be tailored to your use case, you can output as much or as little as you want by adding new members or by commenting existing members out
Decode manually or with decodeRadioDetails.
Documentation needs updating to reflect behavior.