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

Merge RF and RFBridge code #1435

Merged
merged 5 commits into from
Dec 31, 2018
Merged

Merge RF and RFBridge code #1435

merged 5 commits into from
Dec 31, 2018

Conversation

Niek
Copy link
Contributor

@Niek Niek commented Dec 17, 2018

Related issue: #1425

This PR merges the RF and RF Bridge code. No changes in configs/defines. I also took the freedom to rename the module to generic "RF".

@mcspr
Copy link
Collaborator

mcspr commented Dec 17, 2018

I'd suggest to stick to rfbridge.ino as base and keep RFBRIDGE naming when appropriate (per #795 (comment) and general issue of still having either two or one direction rf communication modules)
Plus, it will be much shorter diff.

This does not modify modules behaviour, thus may be safely merged. Later PR can address RFB_DIRECT / RF_SUPPORT, RF_PIN / RFB_PIN etc.

@Niek
Copy link
Contributor Author

Niek commented Dec 18, 2018

I agree with your comment on the filename and diff size - I updated the PR. I kept the function names (like rfbSetup() etc) the same as well. I definitely think it would be wise to identify this as a generic "RF" module though, also in the debug log. In fact the previous RFBridge code did not register as a module, which is strange.

Copy link
Collaborator

@mcspr mcspr left a comment

Choose a reason for hiding this comment

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

lgtm, besides comments
if @xoseperez does not mind rfbridge->rf naming

code/espurna/config/progmem.h Outdated Show resolved Hide resolved
code/espurna/espurna.ino Outdated Show resolved Hide resolved
code/espurna/rfbridge.ino Outdated Show resolved Hide resolved
@Niek
Copy link
Contributor Author

Niek commented Dec 19, 2018

Thanks a lot, I missed that! PR has been updated now.

@Niek
Copy link
Contributor Author

Niek commented Dec 31, 2018

Any chance that this can be merged?

@mcspr mcspr merged commit d299773 into xoseperez:dev Dec 31, 2018
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.

2 participants