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

qualify command 'i' value before setting node id #80

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pb66
Copy link

@pb66 pb66 commented Jul 10, 2015

A stray "i" can attempt to set a node id of "0" and result in a "config save failed" although setting the node id to "0" will of succeeded and the result is no further packets will be processed until a valid node id is set.

A stray "i" can attempt to set a node id of "0" and result in a "config save failed" although setting the node id to "0" will of succeeded and the result is no further packets will be processed until a valid node id is set.
@JohnOH
Copy link
Collaborator

JohnOH commented Jul 11, 2015

Node == 0 is also used to disable interrupts from the radio. The config save failed error message isn't a desirable outcome but interesting point as to which facet ought to be fixed.

setting node id == 0 is used to disable interupts from the radio module, using 32i has the same effect due to bit masking without the vulnerability of accidental activation by value omission assumed to be 0.
@pb66
Copy link
Author

pb66 commented Jul 12, 2015

I wasn't aware of the "interrupt disable" feature so will leave the decision to your better judgement.

I have however amended my code to allow > 0 <33 so that 32i could be used as a "interrupt disable" instead, with a significant reduction in potential accidental activation.

Perhaps the "current configuration" string could be appended to (or even replace) the "config save failed" message to help identify if node 0 is selected, it would be more consistent to provide the "current config"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants