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

[SX127x/RF69] Added setFifoThreshold #1309

Merged
merged 4 commits into from
Nov 3, 2024

Conversation

SzczepanLeon
Copy link
Contributor

This PR add possibility to set custom FIFO threshold level.

@jgromes
Copy link
Owner

jgromes commented Nov 3, 2024

I'm not sure I understand the use-case here. Sicne the FIFO threshold register is also being set from setFifoFullAction, which uses a constant, so it will overwrite whatever was set by setFifoThreshold.

@SzczepanLeon
Copy link
Contributor Author

SzczepanLeon commented Nov 3, 2024

Lets assume that in FSK mode I don't know packet len. It can vary from 12 to 200 (for example), but it is defined in one of first received bytes (lets say it is 3rd received byte).

So my flow is:

  • set infinite mode
  • set Rx interrupt function
  • set FIFO threshold to 6 (less than minimum packet len - in my example 12, but enough to get byte with len value)

when first interrupt is triggered (first part of data is received) I can read packet len and:

  • set new FIFO threshold (depends on remaining data)
  • on interrupt get more data
  • ...

@jgromes
Copy link
Owner

jgromes commented Nov 3, 2024

That makes sense, however, the problem I have is that this will only work if the user calls setFifoThreshold after setFifoFullAction. Ideally those should be independent of each other.

Additionally, because Stream mode is not specific to SX127x, this should also be added to the RF69 class.

@jgromes jgromes self-assigned this Nov 3, 2024
@SzczepanLeon
Copy link
Contributor Author

SzczepanLeon commented Nov 3, 2024

I see one option, remove setFifoFullAction, add setFifoAction with threshold as a parameter with default option Full/MAX
but this is NBC change, also there is no need to execute setDio1Action every time when threshold is changed.

btw, probably CC1101 also can be updated.

@SzczepanLeon SzczepanLeon changed the title [SX127x] Added setFifoThreshold [SX127x/RF69] Added setFifoThreshold Nov 3, 2024
@jgromes
Copy link
Owner

jgromes commented Nov 3, 2024

I see one option, remove setFifoFullAction, add setFifoAction with threshold as a parameter

I was thinking about that too, and backwards compatibility can be addressed by using a default value. The thing I'm more worried about is repeatedly calling setDio1Action, some HALs do not like that.

Probably the easiest option is to just make this limitation explicit in the description of the setFifoThreshold method. That way if users see some unexpected behavior, the docs can point them in the rigth direction (as opposed to digging through the source code).

EDIT: Regarding the CC1101, it doesn't support Stream mode yet, so we don't have to worry about it for now.

@SzczepanLeon
Copy link
Contributor Author

SzczepanLeon commented Nov 3, 2024

What you mean by Stream mode?
CC1101 also support interrupt on GDO when FIFO is filed above X.
obraz
From CC1101 point it is not stream - it is ordinary SPI read triggered by event.
Seems that only attachInterrupt is missing in CC1101 class.

@jgromes
Copy link
Owner

jgromes commented Nov 3, 2024

I meant this - all the FIFO methods are only necessary in cases where the packet length exceeds the size of the radio FIFO. For CC1101, this is not implemented yet - see #1138 for proof it is not as simple as attaching an interrupt ;)

@jgromes jgromes merged commit cb45f4f into jgromes:master Nov 3, 2024
28 checks passed
@jgromes
Copy link
Owner

jgromes commented Nov 3, 2024

Looks good now - merged, thank you for the contribution!

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.

2 participants