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

Make PA ramp up time configurable for SX1262 #1054

Closed
wants to merge 13 commits into from

Conversation

S5NC
Copy link
Contributor

@S5NC S5NC commented Apr 5, 2024

Default behaviour should not be changed.
Backwards-compatible (I would have liked to put rampTime next to power in begin() but this would have been breaking)
If you are okay with this, I can makes the same changes to the SX1261 and SX1268.

@S5NC
Copy link
Contributor Author

S5NC commented Apr 5, 2024

The build fails because LLCC68.h includes_ SX126x.h, the fix would be to also include , uint8_t rampTime = RADIOLIB_SX126X_PA_RAMP_200U in the LLCC68 begin() part of the header file, but I am not familiar with the LLCC68 and don't know if it behaves exactly the same in this regard.

@S5NC
Copy link
Contributor Author

S5NC commented Apr 5, 2024

The build fails because LLCC68.h includes_ SX126x.h, the fix would be to also include , uint8_t rampTime = RADIOLIB_SX126X_PA_RAMP_200U in the LLCC68 begin() part of the header file, but I am not familiar with the LLCC68 and don't know if it behaves exactly the same in this regard.

I have checked and it's implemented the same in both ICs

@jgromes
Copy link
Owner

jgromes commented Apr 5, 2024

The build fails because ...

I have this one weird trick you can try - maybe try building it yourself before pushing to the pull request? You are the one who opened it, it's up to you to make sure it doesn't break the rest of the repository.

Also, despite your original comment, you did add the ramp time to begin methods, which could be considered a breaking change. Why? A simple method e.g. setPaRampTime alongside an additional argument for setOutputPower would have been so much easier.

GitHub sends me an email every time you push and/or comment, and because you both comment AND commit in a stream-of-consciousness style, most of those emails are completely useless to me because by the time I have a chance to reply, everything is changed already. It's incredibly rude to use git commits or issue comments as chat.

@jgromes jgromes marked this pull request as draft April 5, 2024 13:43
@S5NC
Copy link
Contributor Author

S5NC commented Apr 5, 2024

The build fails because ...

I have this one weird trick you can try - maybe try building it yourself before pushing to the pull request? You are the one who opened it, it's up to you to make sure it doesn't break the rest of the repository.

Also, despite your original comment, you did add the ramp time to begin methods, which could be considered a breaking change. Why? A simple method e.g. setPaRampTime alongside an additional argument for setOutputPower would have been so much easier.

GitHub sends me an email every time you push and/or comment, and because you both comment AND commit in a stream-of-consciousness style, most of those emails are completely useless to me because by the time I have a chance to reply, everything is changed already. It's incredibly rude to use git commits or issue comments as chat.

My apologies, I don't have GitHub installed on VS Code, I've been using the online editor. I gave rampTime a default value in all headers, so if it isn't provided, there is no problem. I meant putting rampTime adjacent to power, as this is order it is sent to the SX1262.

I am still getting error: 'virtual int16_t PhysicalLayer::setOutputPower(int8_t)' was hidden [-Werror=overloaded-virtual=], but don't know how to proceed, I am not indenting to change all radios via the PhysicalLayer, only the SX1262 (then later all SX126x). Accepting an unused argument in setOutputPower for non-SX126x ICs is not ideal.

@jgromes
Copy link
Owner

jgromes commented Apr 5, 2024

I am still getting error: 'virtual int16_t PhysicalLayer::setOutputPower(int8_t)' was hidden [-Werror=overloaded-virtual=],

It's because PhysicalLayer expects setOutputPower to have the signature int16_t setOutputPower(int8_t), while you changed it to int16_t setOutputPower(int8_t, uint8_t).

But why are you opening a pull request which doesn't compile and which you cannot fix? If you have a suggestion for a new feature that you can't implement yourself, either learn how to it, or open the appropriate issue.

Closing as implementing this from the scratch myself is easier than fixing your PR.

@jgromes jgromes closed this Apr 5, 2024
@S5NC S5NC deleted the make-ramp-up-time-accessible branch April 16, 2024 17:25
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.

None yet

2 participants