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

Breaking change: Add configurable bitrates, asyncio support, and expose interrupt handler when platform supports #45

Closed
wants to merge 1 commit into from

Conversation

mtai
Copy link

@mtai mtai commented May 30, 2023

Refactor to mirror the code-structure of the original RadioHead Arduino library

  • Adds configurable bitrates with GFSK modulation
  • Adds asyncio support so recv/send are no longer blocking
  • Exposes interrupt handler for platforms that support ISRs
  • Removes register polling on TX/RX in favor of pin IRQ and countio
  • Collapsed examples, user can now uncomment select lines to change from node 1/2

Addresses
#39: Flags exposed on RHPacket
#36: RFM69.handle_interrupt exposed as a possible callback method

…no library

* Adds configurable bitrates with GFSK modulation
* Adds asyncio support so recv/send are no longer blocking
* Removes TX/RX register polling in favor of pin IRQ and countio
* Cleanup RPi Interrupt example, addresses adafruit#36
Copy link
Contributor

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

This library is still in use on M0 boards, where asyncio is not available.Adding async is great, but it also would lock out those boards.

Space is also really tight on those boards, so any increase in library size would make it not fit on those boards.

(Tagging @jerryneedell and @ladyada for interest here. We may discuss this internally as well.)

So we might consider making a new version of this library with your changes, to add async capability, and keeping the old version as is. Another idea is to factor out the async code so that it would only be imported if required, but that requires some careful restructuring of the API. And when the library is frozen in, we need to avoid freezing in the the unused part.

@mtai
Copy link
Author

mtai commented May 31, 2023

Thank you for your consideration!

Some food for thought, it looks like the 32u4 and M0 are relatively older offerings. For newcomers to the ecosystem, they may consider the RP2040 which is beefier and probably a better fit for CircuitPython in general (processing power, ram and flash). In fact, the way the RadioFruit boards are currently priced, it seems like the RP2040 could be the new baseline for projects going forward.

https://www.adafruit.com/product/3076 (32u4) [$24.95]
https://www.adafruit.com/product/3176 (M0) [$24.95]
https://www.adafruit.com/product/5712 (RP2040) [$19.95]

With that in mind, perhaps it makes sense to release a new library version (same name, new major rev number) that is compatible with asyncio+ boards? Would follow typical versioning conventions.

@mtai
Copy link
Author

mtai commented Jun 12, 2023

Hi, any update on this? Alternatively, I can consider releasing this as CircuitPython_RadioHead instead.

@dhalbert
Copy link
Contributor

@mtai Yes, your changes could be a community library (in the community bundle), supported by you. Would that be OK with you?

@ladyada @jerryneedell Does that make sense to you?

@mtai
Copy link
Author

mtai commented Jul 1, 2023

Hi, to follow-up on this, haven't heard from @ladyada or @jerryneedell on what Adafruit would like to do.

Would you prefer updating the Adafruit driver or have this go the community route? I am personally in favor of sending patches upstream so everyone can benefit :)

@jerryneedell
Copy link
Contributor

Sorry -- I missed this discussion. I'll try to review it and comment in the next few days.

@jerryneedell
Copy link
Contributor

A few quick comments/questions: @mtai
This looks like a great improvement to the library in general. Thank you for creating it!
Note that any new library should make it is for the RFM69 / not RFM8x (it would be great to also update the RFM9x library.
Since it cannot support the M0 boards I would lean toward creating a new library. As to whether it is and Adafruit or Community library I guess taht depends on who will maintain it and if Adafruit will continue to maintain the existing adafruit_rfm69 library independently.
Also, is it still usable on a Raspberry Pi with Blinka? are asyncio and countio available with Blinka?
I see that you updated the RPI examples, were they actually tested?

I have not done any testing yet.
I hope to have time to try this out on some hardware in the next few days. Should be fun!

@jerryneedell
Copy link
Contributor

jerryneedell commented Jul 3, 2023

I tried the exmple rfm69_1_quickstart.py on a feather_rp2040_rfm69 but I get

>>> import rfm69_1_quickstart
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "rfm69_1_quickstart.py", line 8, in <module>
  File "adafruit_rfm69.py", line 52, in <module>
ImportError: no module named 'asyncio'

oops -- nevermind -- waking up slowly -- I forgot to install the library -- back to testing....
sorry for the false alarm.

OK -- I now have the QuickStart demo running between a feather rp2040 rfm69 and a feather m4 express with rfm69 featherwing .... progress

rx_packet = rfm69_client.recv_with_ack()
# If no packet was received during the timeout then None is returned.
if rx_packet is None:
continue
Copy link
Contributor

@jerryneedell jerryneedell Jul 3, 2023

Choose a reason for hiding this comment

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

as written, this will bypass the packet transmission if no packet is received.
I changed it to

if rx_packet is not None:
    # Received a packet!
    # Print out the raw bytes of the packet:
    print_packet(rx_packet)

``` so a packet is sent every interval as desired
 

@jerryneedell
Copy link
Contributor

jerryneedell commented Jul 3, 2023

With he change above, the rfm69_2_with_acks demo works.
So far, I am not able to get the rfm69_3_aio_with_ack demo to work. Neither node appears to be receiving ack packets but as demonstrated above, it may well be because I have not set it up properly, still trying....

I am not very fluent in asyncio so I am having some difficulty evaluating this demo since it does not appear to be working correctly for me.

@jerryneedell
Copy link
Contributor

jerryneedell commented Jul 3, 2023

hmm -- some progress
If I run the rfm69_2_with_acks on the feather_m4 and the rfm69_3_aio_with_ack on the feather rp2040, then most of the packets are received and acked. about one in 10 are missed by the feather m4.

I have not been able to get it to work in the opposite direction...yet

@mtai
Copy link
Author

mtai commented Jul 3, 2023

Thanks for all the testing across devices! Unfortunately I only have the ESP32-S3 TFT Feather to test against. I updated the examples based on my library changes.

Re rfm69_2_with_acks and missing packets, I think that's expected due to the overhead of Python managing SPI and the interrupts. I would imagine pure-C bindings would be much more efficient in terms of MCU cycles. I think you would be able to see this if we tested rfm69_throughput (Python) vs a similar setup using Arduino and RadioHead.

Re rfm_3_aio_with_ack, hmm I think I see a bug in example code, try changing L107 to
rfm69_client.address = SRC_ADDRESS

Let me give examples 1-3 another test spin and see if I catch anything.

@jerryneedell
Copy link
Contributor

Thanks for all the testing across devices! Unfortunately I only have the ESP32-S3 TFT Feather to test against. I updated the examples based on my library changes.

Re rfm69_2_with_acks and missing packets, I think that's expected due to the overhead of Python managing SPI and the interrupts. I would imagine pure-C bindings would be much more efficient in terms of MCU cycles. I think you would be able to see this if we tested rfm69_throughput (Python) vs a similar setup using Arduino and RadioHead.

Re rfm_3_aio_with_ack, hmm I think I see a bug in example code, try changing L107 to rfm69_client.address = SRC_ADDRESS

Let me give examples 1-3 another test spin and see if I catch anything.

Yay! -- the fix to L107 worked!! I also now have the code running on esp32s3 boards with rfm69 featherwings.

@jerryneedell
Copy link
Contributor

@mtai Given the extensive changes to the existing library and that the new library cannot support the M0 and the Raspberry Pi/Blinka boards, I think it makes most sense for this to be a new library - something like CircuitPython_RadioHead_RFM69 and you place it in the "Community Bundle". As I mentioned earlier, it would also be great to adapt this to the RFM9x -- That may be best as a separate library rather than trying to support both in one.

Now that I have the demos working here, I'll take a closer look at the code itself.
Thanks for all your work on this.

@jerryneedell
Copy link
Contributor

@mtai How do you execute the rfm69_throughput test?

@mtai
Copy link
Author

mtai commented Jul 4, 2023

Re rfm69_3_aio_with_ack, great! I've had a lot of fun using this example as a ranging test to see how far I can receive signal by looking at RSSI :) I can probably update it to make the BITRATE easier to configure. Its pretty amazing how much farther you can go with lower bitrates.

Re Raspberry Pi/Blinka, are you suggesting you tested it and its not working? If so, can you send me the tracebacks? I have a RPi4 lying around but I've never set it up :o

Re CircuitPython_RadioHead_RFMXX, I'd be happy to adapt this for RFM9x. I don't have a need for LoRa in my projects though. If Adafruit is willing to send me 2 units of the "Adafruit LoRa Radio FeatherWing - RFM95W 900 MHz - RadioFruit", I'd be happy to add support. Re separate library for both modules, I'm thinking I'd include both radios in the same library, pulling the RadioHead common logic into its own module. In other words, 1 library supporting X radios (in this case RFM69 and RFM9X) If people are memory constrained, they can selectively copy the relevant .py files to their lib/ directory.

Re rfm69_throughput, it really depends on what you want to test. This assumes use of TFT boards since I using print(). L4-L33 show my testing and % achieved vs theoretical when SENDER/RECEIVER using rfm_throughput.py on 2 Adafruit ESP32-S3 TFT boards. Based on the all the below instructions, I'll probably iterate on this example to make it easier to configure.

There are 3 dimensions you can test:

  • Bitrate, L130-L143 assumes a TFT board... in the first 5 seconds after boot, click the BUTTON on the Feather until you see the bitrate you want. On non-TFT boards, you can skip and set bitrate between L144 and L147.
  • Packet size (1-60), see L95 and change 1 to anything between 1-60
  • Sends without or with acks, change L99 on the SENDER and L117 on the RECEIVER

What the combinations test:

  • High bitrate, high packet size: Max achievable throughput
  • High bitrate, low packet size: Library overhead

Putting this together:
On the RECEIVER

  1. Comment OUT L147, uncomment L150.
  2. Update L117 if you want to test aio_recv_with_ack or aio_recv.
  3. Set Bitrate using method defined above.

On the SENDER

  1. Check L95 on packet size (1-60 bytes)
  2. Update L99 if you want to test aio_send_with_ack (TCP-like) or aio_send (UDP-like)
  3. Set Bitrate using method defined above.

@jerryneedell
Copy link
Contributor

Thanks for the clarifications on the throughput test.

Regarding the Raspberry Pi, I have not tried it but it uses countio and countio is not supported via Blinka so I assumed it was a non-starter. I could try working around the use of countio. I'll see what I can do with it.

Just to be clear, I don't work for Adafruit. I am just another user and have been involved with the RFM library development over the past several years. I don't speak for Adafuit.

@mtai
Copy link
Author

mtai commented Jul 4, 2023

Technically speaking, on the Raspberry Pi, you don't even need countio, that was used in place of supporting Interrupts on MCUs. To test this hypothesis, I'd try the following... I'll probably need to pull a RPi4 out to actually test this E2E

adafruit_rfm69.py Dummy IRQ Counter class

class AlwaysTrueCounter:
    def __init__(self, irq_pin):
        self.counter = 1

    def reset(self):
        pass

    def deinit(self):
        pass

adafruit_rfm69.py Update L373 to use this class

    self._irq_counter = AlwaysTrueCounter(irq)

rfm69_rpi_interrupt.py Double-check L68

rfm69_client.platform_supports_interrupts = True

@jerryneedell
Copy link
Contributor

jerryneedell commented Jul 5, 2023

FYI, I tried it with the above changes in a RaspBerry Pi
one correction was needed (I think)

class AlwaysTrueCounter:
    def __init__(self, irq_pin):
        self.count = 1

    def reset(self):
        pass

    def deinit(self):
        pass

But the RPi is not receiving any packets. It transmits, bit does not receive.

@jerryneedell
Copy link
Contributor

@mtai Still no luck getting the RPi Interrupts to work. It is not clear to me what is wrong. The example in the current library works with interrupt OK.
I am going to give up on this for now. Please let me know if you figure out the problem.
I would like to do some testing to see if the use of asynicio helps the response time for detecting ACK packets. One problem the current library has is if you try to use "reliable datagram" mode when communication with an Arduino system (using the RadioHead library) then the CircuitPython side often cannot responds fast enough to detect the ACK packet from the Arduino board. It just gets sent too fast. It would be nice if asyncio improves this. I'm not sure when I will be able to get to testing this but it's high on my "to do list".

Have you verified/fixed up the other examples. I would encourage you to create the examples exactly as you are testing them including the pin assignments. I noticed that several of your examples used the CS pin as board.CE1 which is clearly only relevant for Raspberry Pi boards. I think it would be better ti show the connections you actually used.

@jerryneedell
Copy link
Contributor

@dhalbert @mtai
I suggest this PR be closed and then mtai can go ahead and create a new library with a different name for the community bundle. I'll be happy to keep working with it from there and help with adding rfm9x support.

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Oct 9, 2023

I'm going to close this one as per recommendation above.

@mtai please feel free to ready out if you need help getting a new library spun up and submitted for the community bundle.

@FoamyGuy FoamyGuy closed this Oct 9, 2023
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.

4 participants