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

Lora: Make sx126x antenna rx/tx pins optional #1216

Closed
wants to merge 2 commits into from

Conversation

CBJamo
Copy link
Contributor

@CBJamo CBJamo commented Feb 14, 2023

On our hardware, we're using the alt mode function of dio2 to control rx/tx, but not controlling the crystal with dio3, the library currently assumes just the inverse.

The part of this patch addressing the antenna switching is pretty reasonable, but is a breaking change. The part disabling the crystal control is a horrible hack because I'm not sure what the most idiomatic way to approach making that optional is.

@CBJamo CBJamo changed the title Make sx126x antenna rx/tx pins optional Lora: Make sx126x antenna rx/tx pins optional Feb 14, 2023
@ceekdee
Copy link
Contributor

ceekdee commented Feb 15, 2023

Related issues:

Would it be possible to share the specific mcu/sx126x combination for which this dio2/dio3 flexibility is needed so that it can be considered for the consolidated LoRa implementation?

@lulf
Copy link
Member

lulf commented Feb 15, 2023

@ceekdee ^^ This looks good to me, but maybe it's already handled in your rewrite? Maybe it makes sense to merge it for now?

@ceekdee
Copy link
Contributor

ceekdee commented Feb 15, 2023

As stated in the initial comment, this is a breaking change. In particular, the following two examples are affected:

In addition, I assume commenting out DIO3 setup will break support for some mcu/sx126x combinations. A feature could be used to remove this functionality, but I understand there is hesitancy to add features.

@CBJamo
Copy link
Contributor Author

CBJamo commented Mar 10, 2023

Ceekdee's work will certainly supersede this, so I'll close this in favor of #1023

@CBJamo CBJamo closed this Mar 10, 2023
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.

3 participants