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

Using defined debug port and not Serial + CC1101 length mode config options #55

Merged
merged 6 commits into from
Nov 8, 2019

Conversation

phretor
Copy link
Contributor

@phretor phretor commented Nov 6, 2019

No description provided.

@phretor phretor changed the title Using defined debug port and not Serial Using defined debug port and not Serial + CC1101 length mode config options Nov 6, 2019
@phretor
Copy link
Contributor Author

phretor commented Nov 6, 2019

Proposal on how to address #54 in the CC1101 module. I can implement and send more pull request for other modules if this approach is acceptable.

Copy link
Owner

@jgromes jgromes left a comment

Choose a reason for hiding this comment

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

Please also address the following:

  • Add call to CC1101::variablePacketLengthMode() in CC1101::begin() to set the default configuration on startup.
  • In CC1101::packetMode(), use the private variable CC1101::_packetLengthConfig to set the packet mode (currently it would always set it to variable)
  • Add new public methods to keywords.txt (use true tab as separator, not spaces)

src/modules/CC1101.cpp Outdated Show resolved Hide resolved
src/modules/CC1101.cpp Outdated Show resolved Hide resolved
src/modules/CC1101.cpp Show resolved Hide resolved
src/modules/CC1101.cpp Show resolved Hide resolved
src/modules/CC1101.cpp Outdated Show resolved Hide resolved
src/modules/CC1101.cpp Outdated Show resolved Hide resolved
src/modules/CC1101.cpp Show resolved Hide resolved
@phretor
Copy link
Contributor Author

phretor commented Nov 7, 2019

Thanks for the thorough review. I'll handle the comments. Should I just make a new pull request or merge new changes with this one?

@jgromes
Copy link
Owner

jgromes commented Nov 7, 2019

New PR for the new changes in other radio modules seems more appropriate, thanks.

Updating _packetLengthConfig and _packetLength properly
@phretor
Copy link
Contributor Author

phretor commented Nov 7, 2019

Changeset 112110d should address your requests.

Copy link
Owner

@jgromes jgromes left a comment

Choose a reason for hiding this comment

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

Almost there ;)

Also, please address the following:

  • Add call to CC1101::variablePacketLengthMode() in CC1101::begin() to set the default configuration on startup.
  • In CC1101::packetMode(), use the private variable CC1101::_packetLengthConfig to set the current packet mode (currently, it would always set it to variable, even if the user set to fixed prior to calling packetMode())
  • Add new public methods to keywords.txt (use true tab as separator, not spaces)

src/modules/CC1101.cpp Outdated Show resolved Hide resolved
@phretor
Copy link
Contributor Author

phretor commented Nov 8, 2019

Commit a2e64f1 should do it.

@jgromes jgromes merged commit e8702a4 into jgromes:master Nov 8, 2019
@jgromes
Copy link
Owner

jgromes commented Nov 8, 2019

Looks good to me - merged, thanks!

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