Skip to content

fix read_fifo to always define packet before return, Correct typo in dosctring #6

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Dec 13, 2024
Merged

fix read_fifo to always define packet before return, Correct typo in dosctring #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bb4e3b5
fix read_fifo to always define packet before return, Corrct typo in r…
jerryneedell Dec 4, 2024
98c3141
fixup packet=None handling
jerryneedell Dec 6, 2024
87fe934
fix import in rfm9x
jerryneedell Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion adafruit_rfm/rfm69.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -639,10 +639,11 @@ def fill_fifo(self, payload: ReadableBuffer) -> None:
# Write payload to transmit fifo
self.write_from(_RF69_REG_00_FIFO, complete_payload)

def read_fifo(self) -> bytearray:
def read_fifo(self) -> Optional[bytearray]:
"""Read the packet from the FIFO."""
# Read the length of the FIFO.
fifo_length = self.read_u8(_RF69_REG_00_FIFO)
packet = None # return None if FIFO empty
Copy link
Member

Choose a reason for hiding this comment

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

See my other comment re: returning None.

if fifo_length > 0: # read and clear the FIFO if anything in it
packet = bytearray(fifo_length)
# read the packet
Expand Down
7 changes: 4 additions & 3 deletions adafruit_rfm/rfm9x.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
from circuitpython_typing import ReadableBuffer

try:
from typing import Literal
from typing import Literal, Optional
except ImportError:
from typing_extensions import Literal

Expand Down Expand Up @@ -131,7 +131,7 @@ class RFM9x(RFMSPI):
- preamble_length: The length in bytes of the packet preamble (default 8).
- high_power: Boolean to indicate a high power board (RFM95, etc.). Default
is True for high power.
- baudrate: Baud rate of the SPI connection, default is 10mhz but you might
- baudrate: Baud rate of the SPI connection, default is 5mhz but you might
choose to lower to 1mhz if using long wires or a breadboard.
- agc: Boolean to Enable/Disable Automatic Gain Control - Default=False (AGC off)
- crc: Boolean to Enable/Disable Cyclic Redundancy Check - Default=True (CRC Enabled)
Expand Down Expand Up @@ -517,10 +517,11 @@ def fill_fifo(self, payload: ReadableBuffer) -> None:
# Write payload and header length.
self.write_u8(_RF95_REG_22_PAYLOAD_LENGTH, len(payload))

def read_fifo(self) -> bytearray:
def read_fifo(self) -> Optional[bytearray]:
"""Read the data from the FIFO."""
# Read the length of the FIFO.
fifo_length = self.read_u8(_RF95_REG_13_RX_NB_BYTES)
packet = None # return None if FIFO empty
Copy link
Member

Choose a reason for hiding this comment

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

If this can return None, then the type annotation should specify Optional[bytearray]. Is it better to return an empty bytearray? Not entirely sure what the utility of each case is and which would be better.

Copy link

Choose a reason for hiding this comment

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

This might be important since the function that calls read_fifo checks len(packet). Please confirm. But this will probably not work on None. In that case bytearray(0) is probably the correct solution. I will change my test code to set packet = bytearray(0). Unless I report back assume it's working that way.

Copy link

Choose a reason for hiding this comment

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

And we're still left with the mystery about how read_fifo could be called with nothing in the FIFO - which was happening to me the other day. I can't tell if that's happening anymore with my rig. Nonetheless, I think this change and those in the other two files is best practice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If this can return None, then the type annotation should specify Optional[bytearray]. Is it better to return an empty bytearray? Not entirely sure what the utility of each case is and which would be better.

Good catch... I want to think about this a bit -- the calling code has a confusing check:

                    if len(packet) < 5:
                        # reject the packet if it is too small to contain the RadioHead Header
                        packet = None

which will clearly now fail if read_fifo returns None..... I think it best to use your original suggestion to make the return type annotation Optional[bytearray] and refactor the code to always check for packet is not None before checking len(packet). This way read_fifo will be consistent with receive in returning the Optional[bytearray]. Does that make sense?

Copy link
Collaborator Author

@jerryneedell jerryneedell Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I changed it to Optional[bytearray] in the 3 files and in rfm_common.py changed code to avoid testing len(packet) if packet is None. Let me know what you think.

if fifo_length > 0: # read and clear the FIFO if anything in it
packet = bytearray(fifo_length)
current_addr = self.read_u8(_RF95_REG_10_FIFO_RX_CURRENT_ADDR)
Expand Down
3 changes: 2 additions & 1 deletion adafruit_rfm/rfm9xfsk.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,10 +563,11 @@ def fill_fifo(self, payload: ReadableBuffer) -> None:
# Write payload to transmit fifo
self.write_from(_RF95_REG_00_FIFO, complete_payload)

def read_fifo(self) -> bytearray:
def read_fifo(self) -> Optional[bytearray]:
"""Read the data from the FIFO."""
# Read the length of the FIFO.
fifo_length = self.read_u8(_RF95_REG_00_FIFO)
packet = None # return None if FIFO empty
Copy link
Member

Choose a reason for hiding this comment

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

See my previous comment re: returning None.

if fifo_length > 0: # read and clear the FIFO if anything in it
packet = bytearray(fifo_length)
# read the packet
Expand Down
4 changes: 2 additions & 2 deletions adafruit_rfm/rfm_common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ async def asyncio_receive( # noqa: PLR0912
self.crc_error_count += 1
else:
packet = self.read_fifo()
if self.radiohead:
if (packet is not None) and self.radiohead:
if len(packet) < 5:
# reject the packet if it is too small to contain the RAdioHead Header
packet = None
Expand Down Expand Up @@ -503,7 +503,7 @@ async def asyncio_receive_with_ack( # noqa: PLR0912
self.crc_error_count += 1
else:
packet = self.read_fifo()
if self.radiohead:
if (packet is not None) and self.radiohead:
if len(packet) < 5:
# reject the packet if it is too small to contain the RAdioHead Header
packet = None
Expand Down
Loading