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

Fix frequency issue on SX127x (#1368) #1369

Merged
merged 3 commits into from
Jan 3, 2025
Merged

Conversation

CrispyAlice2
Copy link
Contributor

@CrispyAlice2 CrispyAlice2 commented Jan 3, 2025

The SX127x requires that the LSB of the frequency register (Frf) be written in order to change the frequency, but the library was using a SPI writing function that checked for overwrites. If the LSB did not change, the new frequency would be written in successfully, but the frequency would stay at the module's default.

I changed it to use the lower level SPI writing function with no overwrite protection if there was no difference between the old and new contents for the LSB.

I have this commit working on my RFM96W board connected to an Arduino Nano Every, but I have not tested on any other platforms.

@jgromes
Copy link
Owner

jgromes commented Jan 3, 2025

Thank you for the contribution! Although, I'm not sure this is the right way to fix this because we may encounter more bugs like this, where the radio is sensitive to register access. I think the better way would be to add a flag to SPIsetRegValue, e.g. bool force = false that will always write the register if set. Then from SX127x::setFrequencyRaw we can set force=true to always write it (and perhaps in other places if those come up).

@CrispyAlice2
Copy link
Contributor Author

Do you think it is worth optimizing out the initial register read when force is enabled?

@jgromes
Copy link
Owner

jgromes commented Jan 3, 2025

That needs to be kept, since we might only want to change some of the bits. The only point at which we could drop it is if force is enabled and all bits will be modified, but that seems like too much of an edge case now. Let's keep this PR to just fix this one thing ;)

@jgromes jgromes merged commit 680e88c into jgromes:master Jan 3, 2025
29 of 30 checks passed
@jgromes
Copy link
Owner

jgromes commented Jan 3, 2025

Thank you so much - all looks good, the failed CI run is unrelated and can be ignored.

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