adafruit / Adafruit_CircuitPython_RFM69

CircuitPython module for the RFM69 series of 433/915 mhz packet radios.
MIT License
31 stars 28 forks source link

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

Closed jerryneedell closed 10 months ago

jerryneedell commented 10 months ago

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 commented 10 months ago

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

tottaka commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

@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!