-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add High Power Feature for RFM69HCW #41
base: master
Are you sure you want to change the base?
Conversation
* Added tx_level * Running Workflow * Fixed the formatting * Added test, and initial implementation for the tx_level method * Reworked the tx_power method, and split it into two methods tx_level and tx_level_high_pwe * fixed the formatting * Fixed lint error by implementing clamp fuctions in the tx_level methods * Removed the branch from the CI branched
Please feel free to provide any feedback. I just wanted to get your input sooner than later, I tested it with two RFM69HCW radios and an RP Pico, and it seems to be working great. I'm not sure if there is a way to ID if the radio is a high power model or not, but if so it might be worth returning an err if the user tries to call tx_level_high_pwr on a radio that doesn't support high pwr. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
sorry for the delay, it slipped under my radar. I have few comments.
@@ -271,11 +273,21 @@ where | |||
|
|||
self.reset_fifo()?; | |||
|
|||
// If the trasnmit power is over 18, turn on boost mode | |||
if self.tx_pwr >= 18 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 18 should be constant. That applies to all checks like this.
@@ -353,6 +376,46 @@ where | |||
self.write(Registers::TestLna, boost as u8) | |||
} | |||
|
|||
pub fn tx_level(&mut self, power_level: i8) -> Result<(), Espi> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function is missing documentation comment that would describe the limitation of power_level
, also tx_level
isn't very descriptive name, I would suggest tx_power
instead WDYT?
@@ -353,6 +376,46 @@ where | |||
self.write(Registers::TestLna, boost as u8) | |||
} | |||
|
|||
pub fn tx_level(&mut self, power_level: i8) -> Result<(), Espi> { | |||
let adjusted_power = power_level.clamp(-18, 13); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both -18
and 13
should be constants.
pub fn tx_level(&mut self, power_level: i8) -> Result<(), Espi> { | ||
let adjusted_power = power_level.clamp(-18, 13); | ||
|
||
let reg_value = PaOptions::Pa0On as u8 | (adjusted_power + 18) as u8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
18
should be constant.
Ok(()) | ||
} | ||
|
||
pub fn tx_level_high_pwr(&mut self, power_level: i8) -> Result<(), Espi> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above with the function documentation and the name. Also throughout the whole function, please convert all "magic" numbers into constants.
Ok(()) | ||
} | ||
|
||
fn enable_boost_mode(&mut self) -> Result<(), Espi> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All private function are defined below public ones, please move those two there as well.
As for the detection, I have unfortunately no idea if it's possible. I wonder what happens if you set wrong power level on non-high power RFM. Would it destroy the chip? If that might be the case maybe we should mark the high power function as unsafe and add warning to the documentation. |
#4 Add High Power
Datasheet
Added tx_level and tx_level_high_pwr methods to the rfm69 struct
Added enable_boost_mode and disable_boost_mode methods to enable and disable the high power transmit settings during transmissions where the tx level is >= 18