Attempt to fix compat_mode_pigpio #115
Conversation
|
Interested in what the Travis CI will output. I don't have it setup (yet) |
-implement for pigpio
TojikCZ
force-pushed
the
fix_compat_mode_pigpio
branch
from
d719bb9 to
f747f32
Compare
|
rebased on master, squashed ugly fix commits |
|
Feeling pretty ignored over here. Guess I'm deploying with my fork stuck in the time I made it. |
@TojikCZ sorry for the late response (especially since this was your first contribution). I have lots of open source projects (plus other obligations and interests as well), and sometimes a GitHub notification e-mail gets overlooked.
A friendly ping after a few weeks of inactivity usually helps to bring the issue to my attention. Your patch looks pretty simple, so I'll take a look right now. |
| if sys.version_info.major < 3: | ||
| from time import clock as now | ||
| else: | ||
| from time import perf_counter as now |
There was a problem hiding this comment.
Looking at this Python 2 compat code, I think it's time to drop Python 2 support in the next release. However, for now, let's keep it since it still works.
| Default: False | ||
| compat_mode_wait_time: Minimum time to pass between sends. | ||
| if zero, turns off compat_mode | ||
| Default: ``0.001`` seconds. |
There was a problem hiding this comment.
Since compat_mode=False and compat_mode_wait_time=0 are essentially the same, can we merge the two parameters?
There was a problem hiding this comment.
Yes, I'd like to. But there are two problems with having just the wait amount.
We'll lose backwards compatibility. That's ok. I can do an assert that tells would be users what to do instead.
There would be no default for compat_mode "on". The default amount would have to be sourced from something like DEFAULT_COMPAT_WAIT_TIME constant or the docs.
It's undoubtedly cleaner and gets rid of invalid configs like compat_mode=False with a high compat_mode_wait_time value. Your pick 😉
RPLCD/lcd.py
Outdated
| raise ValueError('Invalid data bus mode: {}'.format(self.data_bus_mode)) | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Two stray empty lines that can be removed.
RPLCD/pigpio.py
Outdated
|
|
||
| # Record the time for the tail-end of the last send event | ||
| if self.compat_mode: | ||
| self.last_send_event = now() |
There was a problem hiding this comment.
It's a bit weird that the compat mode is something that is supported in the base class, but that implementation details need to be handled in the subclass. How about this?
- Add two functions in the base class called
_compat_mode_wait()and_compat_mode_record_send_event(). - In the two subclasses, never explicitly check for
self.compat_mode, instead simply call those two functions (which are a no-op if compat mode is not enabled).
Another benefit of this is that the Python 2 compat importing of now is only needed once (in the base class).
|
Sorry, I got a taste of how missable github notifications can be shortly after posting this 😅 |
TojikCZ
force-pushed
the
fix_compat_mode_pigpio
branch
from
f747f32 to
868d0d5
Compare
Remove busy-wait Fix initialization bug
TojikCZ
force-pushed
the
fix_compat_mode_pigpio
branch
from
868d0d5 to
61ed40b
Compare
3 participants
Hi, after discovering the pull request that was supposed to add the compat mode to the pigpio side i included my changes to make the sleep amount tune-able and to make it stop busy waiting. The wait gets less precise but I don't think that's a big deal, sleep overshoots by 0.15 nano seconds on my pi. There is a lot of sleep requests shorter than that and the code works regardless.
I have another thing ready for a pull request. I made the caching optional as my setup produces too many errors for me to trust it with displaying the text right on the first try.
This is my first time trying to contribute to an open-source project, so criticize everything i do, so I'll learn something new :)