Skip to content

Fix setting of the Power amplifiers when power boost is enabled #48

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

Merged
merged 4 commits into from
Nov 27, 2023

Conversation

jerryneedell
Copy link
Contributor

fixes #47

Fixes previous error in setting power amplifiers.
Also fixes reporting of tx_power for levels 14-17
Turns off Overload Current protection when tx_power is >18 as specified int he Data sheet.

Tested on feather_m0_rfm69 and Raspberry Pi bonnet.

Additional testing welcome.

This does add about 50 bytes of code to the EN build for the feather_m0_rfm69 but it still fits....

@jerryneedell jerryneedell mentioned this pull request Nov 17, 2023
@jerryneedell
Copy link
Contributor Author

jerryneedell commented Nov 17, 2023

It would be good for someone else with rfm69 hardware to test and review this. Thanks.

@jerryneedell jerryneedell requested a review from a team November 18, 2023 15:50
@tottaka
Copy link

tottaka commented Nov 19, 2023

Why even bother checking if high power is enabled in the disable_boost function? If boost needs to be disabled then it should be disabled in any/all possible cases.

Also, I feel like the code could be even smaller if the high_power property is completely removed in favor of simply using tx_power or similar. Please do correct me if I'm missing something

@jerryneedell
Copy link
Contributor Author

jerryneedell commented Nov 19, 2023

Why even bother checking if high power is enabled in the disable_boost function? If boost needs to be disabled then it should be disabled in any/all possible cases.

Also, I feel like the code could be even smaller if the high_power property is completely removed in favor of simply using tx_power or similar. Please do correct me if I'm missing something

The reason for the check is that for non HCW boards the RegTestPa1 and RegTestPa2 registers do not exist. See the SX1231 data sheet https://cdn-shop.adafruit.com/product-files/3076/sx1231.pdf.
I put the check for "high_power" in the function so that it only appears once instead of before every call to disable_boost().

@tottaka
Copy link

tottaka commented Nov 19, 2023

Why even bother checking if high power is enabled in the disable_boost function? If boost needs to be disabled then it should be disabled in any/all possible cases.
Also, I feel like the code could be even smaller if the high_power property is completely removed in favor of simply using tx_power or similar. Please do correct me if I'm missing something

The reason for the check is that for non HCW boards the RegTestPa1 and RegTestPa2 registers do not exist. See the SX1231 data sheet https://cdn-shop.adafruit.com/product-files/3076/sx1231.pdf. I put the check for "high_power" in the function so that it only appears once instead of before every call to disable_boost().

Ah okay that makes sense now... Anyways, the changes you've made seem good to me

@tottaka
Copy link

tottaka commented Nov 21, 2023

@jerryneedell disable boost in constructor could just call the disable_boost function instead I think to remove a few extra bytes of code. In fact it probably should be done this way because there is no high_power check in the constructor when disabling boost so for non-HCW boards those operations wont work since they don't have RegTestPa1 and RegTestPa2 registers.

Edit: Actually, since the device state is immediately set to idle in the constructor which in-turn disables boost anyways, the disable boost in the constructor can be completely removed, right?

@jerryneedell
Copy link
Contributor Author

@jerryneedell disable boost in constructor could just call the disable_boost function instead I think to remove a few extra bytes of code. In fact it probably should be done this way because there is no high_power check in the constructor when disabling boost so for non-HCW boards those operations wont work since they don't have RegTestPa1 and RegTestPa2 registers.

Edit: Actually, since the device state is immediately set to idle in the constructor which in-turn disables boost anyways, the disable boost in the constructor can be completely removed, right?

Agreed! I will remove it from the constructor. Thanks!

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.

Thanks!!

@dhalbert dhalbert merged commit 30e8723 into adafruit:main Nov 27, 2023
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Nov 28, 2023
Updating https://github.com/adafruit/Adafruit_CircuitPython_HUSB238 to 1.0.1 from 1.0.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_HUSB238#3 from millercommamatt/patch-2
  > example typo

Updating https://github.com/adafruit/Adafruit_CircuitPython_RFM69 to 2.1.18 from 2.1.17:
  > Merge pull request adafruit/Adafruit_CircuitPython_RFM69#48 from jerryneedell/jerryn_power

Updating https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT to 7.5.3 from 7.4.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#186 from vladak/var_header_vs_bytearray
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#183 from vladak/wait_for_msg_pkt_type
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#182 from vladak/time_monotonic_ns
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#188 from vladak/user_data_public

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
@jerryneedell jerryneedell deleted the jerryn_power branch November 28, 2023 21:02
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.

set_boost oversight?
3 participants