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

extend PhyTxRx to consider antenna gain and max power #159

Merged
merged 1 commit into from Nov 9, 2023

Conversation

lthiery
Copy link
Collaborator

@lthiery lthiery commented Nov 5, 2023

Resolves #150

device/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@lulf lulf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why couldn't this be just added to PhyRxTx with default impls to avoid the extra generic arg?

@lthiery lthiery changed the title create BoardEirp trait extend PhyTxRx to consider antenna gain and max power Nov 6, 2023
@lthiery lthiery force-pushed the lthiery/max-power branch 2 times, most recently from ff2ccf9 to b3415fb Compare November 6, 2023 16:03
@lthiery
Copy link
Collaborator Author

lthiery commented Nov 6, 2023

Why couldn't this be just added to PhyRxTx with default impls to avoid the extra generic arg?

Thanks for the feedback! I think you're right and I tried it out combined with @plaes 's const generic suggestion.

Copy link
Collaborator

@plaes plaes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I do not like with current design is that it's somewhat error-prone as it has to be "manually" adjusted every time mac.{send,join_otaa} creates TxConfig object.

Ideally it should be already adjusted whenever this object is craeted which happens in only two places:

  • join_otaa
  • send
    Or alternatively it should happen when this instance is used (one possibility could be adding extra optional argument for radio.tx(...) in lora-phy)?

device/src/async_device/lora_radio.rs Outdated Show resolved Hide resolved
device/src/async_device/radio.rs Outdated Show resolved Hide resolved
device/src/async_device/radio.rs Outdated Show resolved Hide resolved
@lthiery
Copy link
Collaborator Author

lthiery commented Nov 8, 2023

What I do not like with current design is that it's somewhat error-prone as it has to be "manually" adjusted every time mac.{send,join_otaa} creates TxConfig object.

Thanks for the idea @plaes. Your point is why my first iteration passed a const generic to the Mac struct initially, but @lulf 's feedback made me think that the mental load of yet another trait seemed unnecessary and I liked the UX of the trait associated const.

I was going to do Mac<R::MAX_RADIO_POWER,R::ANTENNA_GAIN> but that depends on generic_const_exprs which can only be enabled in nightly. Even with that feature enabled, I couldn't quite get it to work and the compiler was telling me that the expressions were not properly constrained...

Therefore, the current solution is to use the trait constants to instantiate the Mac with the appropriate limits. This at least passes the responsibility the adjustment to join_otaa and send making it harder to make a mistake in the future.

device/src/mac/mod.rs Outdated Show resolved Hide resolved
lulf
lulf previously approved these changes Nov 9, 2023
@lthiery lthiery merged commit d5df83c into main Nov 9, 2023
2 checks passed
@lthiery lthiery deleted the lthiery/max-power branch November 9, 2023 15:54
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.

DBM values in Australia and America block usage of low-power ChipType::SX1261
3 participants