dhylands / python_lcd

Python based library for talking to character based LCDs.
MIT License
298 stars 116 forks source link

Broken CPython support #36

Closed clach04 closed 2 years ago

clach04 commented 2 years ago

Thanks for making this library available! From reading through the docs/code looks like the intent is support both CPython and MicroPython?

I just tried this on my SBC with CPython (GPIO I²C port expander (using a PCF8574) to a Hitachi HD44780 controller) and got errors in the sleep code. It looks like this is micropython specific?

I have a quick hack so I'm up and running, but not sure what direction to take here for final fix:

(py38) pi@rock64lcd:~/py/python_lcd$ git diff
diff --git a/lcd/lcd_api.py b/lcd/lcd_api.py
index 61945d5..928c75c 100644
--- a/lcd/lcd_api.py
+++ b/lcd/lcd_api.py
@@ -205,4 +205,5 @@ class LcdApi:

     def hal_sleep_us(self, usecs):
         """Sleep for some time (given in microseconds)."""
-        time.sleep_us(usecs)
+        #time.sleep_us(usecs)  # appears to be a micropython specific function
+        time.sleep(usecs / 1000000)

My instinct is to support CPython time.sleep first and only use time.sleep_us if micropython has no support for it. I'm unclear on the micropython support though :(

We could monkey patch in a sleep_us but it feels a bit dirty.

Thoughts before I hack in a monkey patch and open a PR?

clach04 commented 2 years ago

Error details:

Traceback (most recent call last):
  File "my_demo.py", line 40, in <module>
    lcd.custom_char(0, happy_face)
  File "/home/pi/py/python_lcd_clach04/lcd/lcd_api.py", line 170, in custom_char
    self.hal_sleep_us(40)
  File "/home/pi/py/python_lcd_clach04/lcd/lcd_api.py", line 208, in hal_sleep_us
    time.sleep_us(usecs)
AttributeError: module 'time' has no attribute 'sleep_us'
dhylands commented 2 years ago

The intent is that lcd_api.py should be independent of the platform. So it shouldn't have a sleep or a sleep_us in it. sleep_us is used so that you don't need to have floating point enabled (not all micropython port provide support for floating point operations).

Unfortunately, the sleep stuff came in fairly late in the development, so I added a "default" implementation in lcd_api.py so as not to break the existing implementations.

Specific hal layers can override this if desired. circuitpython, for example, also doesn't support sleep_us so it provides a hal_sleep_us function like this: https://github.com/dhylands/python_lcd/blob/4376476d7753894cf0a5093a2f53839366b131a7/lcd/circuitpython_i2c_lcd.py#L84-L86

It looks like i2c_lcd.py (which is intended for CPython use) should provide a similar hal_sleep_us function.

dhylands commented 2 years ago

I think that adding some comments to the hal_sleep_us function implemented in lcd_api.py would also help.

clach04 commented 2 years ago

Thanks for taking the time to explain this, really helpful. I posted a PR that resolves the issue and should help with other platforms. It may benefit from further comments/hints.