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

Support SX126x RF switch controlled by MCU #5

Merged
merged 3 commits into from
Jul 23, 2020

Conversation

andysan
Copy link

@andysan andysan commented May 5, 2020

This PR contains a series of changes that add support for SX126x radio state tracking from Zephyr. These changes are needed to support RF switches that are wired to MCU GPIOs instead of directly to the radio.

NOTE: Depends on #4 which adds build configs for SX126X.
NOTE: There isn't a PR for the Zephyr side yet since the base SX126x driver hasn't been merged. See https://github.com/andysan/zephyr/tree/sx126x-rf-switch for my development branch.
NOTE: This is a dependency for zephyrproject-rtos/zephyr#26611

@Mani-Sadhasivam
Copy link
Member

@andysan It is always recommended to mention the corresponding Zephyr PR for reviewers to make sure the change builds.

@andysan
Copy link
Author

andysan commented May 7, 2020

@andysan It is always recommended to mention the corresponding Zephyr PR for reviewers to make sure the change builds.

There isn't a PR for the Zephyr side yet since I wanted to make sure the base support is merged to avoid complicating the review. I have a branch here though if you are interested: https://github.com/andysan/zephyr/tree/sx126x-rf-switch

@andysan
Copy link
Author

andysan commented Jul 2, 2020

@Mani-Sadhasivam I have rebased this and posted the corresponding Zephyr patches.

@Mani-Sadhasivam
Copy link
Member

@andysan Thanks for the PR! I like the idea of controlling the RF switch using MCU GPIOs but we should really try to minimize the delta between upstream LoRaMac-Node repo and ours. So I'd suggest you to post the PR to the upstream repo by having a generic solution which could work for other projects as well. So you need to consider removing the __ZEPHYR__ flag and use __weak attribute and also introducing an API for toggling the RF switches manually in SX126xSetOperatingMode.

I know it is a bit of work for you but if we keep adding stuff like this in our fork then soon it will become a mess.

@andysan
Copy link
Author

andysan commented Jul 5, 2020

I have raised or updated the following upstream issues related to this PR:

@andysan
Copy link
Author

andysan commented Jul 6, 2020

The necessary changes have now been merged upstream. What’s the policy for pulling in new changes? Should I just cherry pick them? Are we on a release branch/tag? Or should I rebase the Zephyr changes on top of master?

…tTxContinuousWave and SX126xSetTxInfinitePreamble functions
@andysan
Copy link
Author

andysan commented Jul 6, 2020

Ended up cherry-picking the relevant changes. Let me know if I should do something else.

…entation

Removed SX126xDbgPinTxWrite and SX126xDbgPinRxWrite functions from public API
@andysan
Copy link
Author

andysan commented Jul 9, 2020

Ended up cherry-picking the relevant changes. Let me know if I should do something else.

UPDATE: Remove re-ordering commit that was reverted on upstream since it prevented the radio from waking up correctly.

@andysan
Copy link
Author

andysan commented Jul 9, 2020

Ping @Mani-Sadhasivam . Is it acceptable to merge this PR with the commits backported from upstream?

@Mani-Sadhasivam
Copy link
Member

@andysan Sorry for the late reply! Instead of cherry picking the commits, can you please rebase this to master?

@Mani-Sadhasivam Mani-Sadhasivam self-assigned this Jul 15, 2020
@andysan
Copy link
Author

andysan commented Jul 15, 2020

@andysan Sorry for the late reply! Instead of cherry picking the commits, can you please rebase this to master?

Would it be a better idea to use a release tag and cherry pick the change? I'm a bit nervous about not using a release here.

@Mani-Sadhasivam
Copy link
Member

Mani-Sadhasivam commented Jul 16, 2020

Would it be a better idea to use a release tag and cherry pick the change? I'm a bit nervous about not using a release here.

Ack. Sorry for that comment (bit sleepy last night). Let's move onto 4.4.4 release and cherry pick these changes along with a few we are carrying in our repo.

@andysan
Copy link
Author

andysan commented Jul 18, 2020

Would it be a better idea to use a release tag and cherry pick the change? I'm a bit nervous about not using a release here.

Ack. Sorry for that comment (bit sleepy last night). Let's move onto 4.4.4 release and cherry pick these changes along with a few we are carrying in our repo.

I had a look at that this would entail. Moving to 4.4.4 changes the signature of a couple of functions in the SX1276 radio. It's also unclear exactly how we deal with branching in this module. Rebasing is probably not an option since that would leave some changes dangling and subject to garbage collection (which would break older versions of Zephyr). I would merge the new tag, but that makes it harder to track which changes are Zephyr-specific.

Can we merge these changes now and figure out how we move to 4.4.4 separately? They aren't in the 4.4.4 release anyway.

@Mani-Sadhasivam
Copy link
Member

Would it be a better idea to use a release tag and cherry pick the change? I'm a bit nervous about not using a release here.

Ack. Sorry for that comment (bit sleepy last night). Let's move onto 4.4.4 release and cherry pick these changes along with a few we are carrying in our repo.

I had a look at that this would entail. Moving to 4.4.4 changes the signature of a couple of functions in the SX1276 radio. It's also unclear exactly how we deal with branching in this module. Rebasing is probably not an option since that would leave some changes dangling and subject to garbage collection (which would break older versions of Zephyr). I would merge the new tag, but that makes it harder to track which changes are Zephyr-specific.

Can we merge these changes now and figure out how we move to 4.4.4 separately? They aren't in the 4.4.4 release anyway.

Hmm. Okay.

Copy link
Member

@Mani-Sadhasivam Mani-Sadhasivam left a comment

Choose a reason for hiding this comment

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

LGTM

@Mani-Sadhasivam
Copy link
Member

@carlescufi Could you please merge this PR?

@carlescufi carlescufi merged commit 3f545d7 into zephyrproject-rtos:master Jul 23, 2020
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.

4 participants