fix read_fifo to always define packet before return, Correct typo in dosctring #6
Conversation
| @@ -521,6 +521,7 @@ def read_fifo(self) -> 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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If this can return
None, then the type annotation should specifyOptional[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?
There was a problem hiding this comment.
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.
| @@ -567,6 +567,7 @@ def read_fifo(self) -> 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 | |||
There was a problem hiding this comment.
See my previous comment re: returning None.
| @@ -643,6 +643,7 @@ def read_fifo(self) -> 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 | |||
There was a problem hiding this comment.
See my other comment re: returning None.
|
@tekktrik When you have a chance would you take a look at my attempt to resolve this. Let me know if it looks OK. |
Updating https://github.com/adafruit/Adafruit_CircuitPython_RFM to 1.0.3 from 1.0.2: > Merge pull request adafruit/Adafruit_CircuitPython_RFM#6 from jerryneedell/jerryn_read_fifo > Merge pull request adafruit/Adafruit_CircuitPython_RFM#3 from FoamyGuy/docs_container_version_fix Updating https://github.com/adafruit/Adafruit_CircuitPython_LED_Animation to 2.10.0 from 2.9.3: > Merge pull request adafruit/Adafruit_CircuitPython_LED_Animation#121 from FoamyGuy/rainbow_period_property Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Updated download stats for the libraries
fixes #5
fixes #4
I was not able to reproduce the OP issue for #5 but the fix appears to have worked for the OP and is better coding practice.
Tested with rfm69 and rfm9x bonnets on Raspberry Pi.