-
Notifications
You must be signed in to change notification settings - Fork 21
update contributed sonar library to make even better! #1
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good improvements! Just a couple more suggestions.
adafruit_hcsr04.py
Outdated
if isinstance(trig_pin, str): | ||
trig_pin = getattr(board, trig_pin) | ||
if isinstance(trigger_pin, str): | ||
trigger_pin = getattr(board, trigger_pin) | ||
if isinstance(echo_pin, str): | ||
echo_pin = getattr(board, echo_pin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about removing this. It adds a second way to do something that will be confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally OK - will do!
adafruit_hcsr04.py
Outdated
# method on instantiation. | ||
pass | ||
if self._trig is not None: | ||
return self._dist_two_wire() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it throw the runtime exception? A missing return is return None
implicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at this time we only support two_wire so ill just remove the if() and we'll add one-wire later if i can find a PING sensor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
keep _USE_PULSEIO false for NotImplementedError
now uses the
distance
property for standard cm output, and works with both pulseio and non-pulseio boards, like Raspberry Pi (e.g. linux)