robert-hh / Onewire_DS18X20

Classes for driving the DS18x20 sensor with the onewire protocol for Pycom MicroPython
12 stars 6 forks source link

Compatibility with Genuine MicroPython #10

Closed amotl closed 4 years ago

amotl commented 4 years ago

Hi there.

This module is coming from a time when neither Genuine MicroPython nor Pycom MicroPython had native drivers for 1-Wire/DS18X20 sensors so they had to be implemented in pure-Python.

The advent of [1] changes that situation, so @robert-hh and me are striving towards making the Python part blend into each other seamlessly.

In other words, the pure-Python library should be made API compatible with [2].

With kind regards, Andreas.

[1] https://github.com/pycom/pycom-micropython-sigfox/pull/356 [2] https://github.com/micropython/micropython/tree/master/drivers/onewire

amotl commented 4 years ago

For keeping both versions around within a single repository, I propose putting them in different directories instead of calling them xxx_new.py.

Within setup.mk, we called them xxx_python.py vs. xxx_native.py. In order to map this idea with directories, we might just call them python vs. native.

However, we might want to skip the native part altogether when adding additional convenience functionality through an adapter class as outlined within #11.

amotl commented 4 years ago

Right now, we've introduced a compatibility adapter called DS18X20NativeDriverAdapter. After getting the lowlevel drivers compatible with each other, we will be able to get rid of that altogether.

amotl commented 4 years ago

In order to make both libraries compatible with each other, we should also send a PR to Genuine MicroPython which adds functionality to the convert_temp method to start the conversion on a specific sensor device by obtaining a rom argument like the read_scratch and write_scratch methods.

amotl commented 4 years ago

We should also take care not to communicate with the hardware within the object constructor already. This has already bitten the C-driver for the HX711 sensor which we recently refactored not to do this anymore through https://github.com/bogde/HX711/pull/123.

amotl commented 4 years ago

We should also take care not to communicate with the hardware within the object constructor already.

I can see this is currently the case through

https://github.com/robert-hh/Onewire_DS18X20/blob/e73b0c1df841ac90c662fd2d00cd4efb1bb82108/ds18x20.py#L15-L17

As we got rid of invoking the scan() method from within the constructor, we might think about putting this into a specific method as well. Is it actually about activating the parasite power mode the DS18B20 is capable of?

Elsewhere, this is done through a readPowerSupply method.

robert-hh commented 4 years ago

Already done, but not optimal. Because then the mode is only known to the driver if the method has been called. And adding another check in each and every call is nasty too.

amotl commented 4 years ago

As the driver API is already compatible with Genuine MicroPython, we might want to close this issue and divert discussions about different topics into another issues.

Thanks a bunch for all your work on this, @robert-hh!