kplindegaard / smbus2

A drop-in replacement for smbus-cffi/smbus-python in pure Python
MIT License
243 stars 68 forks source link

Support of multiple 'engines'? #71

Open mefistotelis opened 3 years ago

mefistotelis commented 3 years ago

Since KMD supports currently only SMBus 2.0, and the message length is restricted even more by length being included in the bytes limit, I had to implement SMBus using I2C commands.

Maybe smbus2 itself should include such implementation? There could be a selection option on whether messages should be constructed by kernel or by python. I'd call it 'engine' selection, or 'api' selection.. not sure what's the best name.

This would also allow support of other 'engines' in the future - for example, for USB-to-I2C bridges, which typically are visible as USB HID device, there is a library smbusb. That library can also be used on Windows. I might work on Python bindings for it in the future.

So available 'engines' may depend on OS - the list should be dynamic.

mefistotelis commented 3 years ago

Looking more into the code, I think the only way to do that would be to prepare multiple versions of SMBus class, ie. SMBusOnKmdSmbus, SMbBsOnKmdI2C, SMBusOnSmbusb. Then either allow the user to create specific SMBus.*, or make a user-callable selection function which defines main SMbus class as one of these.

As I now understand more about SMBus, I am also not a fan of how (write|read)_i2c_block_data is implemented. it forces length of the packet as first byte on write, but doesn't remove that byte on read - so the functions are kind of non-symmetrical. And adding the length automatically, defies the purpose of write_i2c_block_data kernel call - it just becomes another write_block_data, only relying less on kernel and more on Python code. I understand this is because OG py-smbus module implemented it like this. But this is still a bad idea here, as it was in py-smbus.

Maybe it's not that good of an idea to use this API as a base for the future.

EDIT: my current implementation: https://github.com/mefistotelis/smbus2

kplindegaard commented 3 years ago

I'm going to try to give some general feedback here. Let me share some of my thoughts and considerations I made when the lib was started upon. My primary goal was to find a Python lib I could include in a Flask-based API server application that was going to deployed on a large number of Raspberry Pi's and delivered to clients. Simple speaking, the criteria were as follows:

None of the existing SMbus/i2c libraries I could find ticked all the boxes. Therefore smbus2 came into being, and the first version was very "down to Earth" in the sense that it served what I needed at the time. Several contributors chipped in and completed the missing functionality plus streamlined docs and builds over the years, and I am very grateful for that :).

It was intentional to keep the library small. Error handling is very minimalistic, and it was convenient to reuse the interface of the at the time standard smbus library frequently used in tutorials and examples. The function names and parameter list in the SMBus class come from that. Kind of practical for me, but it also lowered the threshold for more general adoption. These considerations combined justified a single "engine": Mapping directly to ioctl using the Linux kernel's struct definitions would suffice.

With respect to support for multiple engine alternatives, there are several directions you may want to consider. First, is the "interface" as defined in the SMBus class worth keeping? It's not written in stone, although it is convenient. Second, could some form of dependency injection be exploited instead of having X number of SMBus class variants? I'm not a Python-guru, but to me that sounds like a more attractive option. Third: I should also say that I have my own "adapter" on top of smbus2 where I can choose between several "read/write large blocks of data" strategies. One for reading/writing 32 bytes at the time directly mapping to read_i2c_block_data, another for arbitrarily large blocks (keeping track of offsets and what not) mapping to i2c_rdwr, and even one that performs byte-wise read/write operations in case that's what the i2c-slave is capable of handling. And yes, it has happened that I have had to use that.

Not sure any of this will help you directly in your quest, but I hope it was semi-useful anyway. In my humble opinion I think the simplicity and low threshold of the smbus2 has served it well so far. Whether to extend in multiple directions or not, well, I don't really have a strong opinion on that except that I know that both shoehorning as well taking off on a tangent can be painful experiences in the long run. That being said, I definitely don't want to dismiss your initiative either!

mefistotelis commented 3 years ago

I agree complicating the library might not be the best idea. I am now leaning towards just creating another library which relies in I2C calls - so if smbus2 doesn't work for someone, he can just switch to smbus2-i2c or whatever, without affecting the API too much.

So the selection between smbus2, smbus2-i2c and smbusb would be effectively done on the application side. The issue is - smbusb has different API, which makes writing the adapter complicated.

I definitely want to make an app which would be able to choose between these, just not sure how (and where) to make the selection for best results.

At the moment I got preempted from the work on SMBus and batteries, but I'm hoping to get back to it.

Btw, I have a SMBusPureOnI2C class implemented on my fork; though as I said, I'm leaning towards making a new module rather than adding 2nd class to smbus2.

Though you might want to take this patch from my branch: https://github.com/mefistotelis/smbus2/commit/e4b611bd71f72c373a53a422865d9979fc9fa015

DavidEGrayson commented 1 year ago

For what it's worth, I recently developed a library that supports multiple I2C "engines". I wanted the library to work on smbus2 in Python 3 and also the machine.I2C class in MicroPython. The result is the MotoronI2C class linked below, which detects what type of bus object you give it and then it enables the proper routines for talking to that object.

https://github.com/pololu/motoron-python/blob/master/motoron.py

It would be nice if the APIs of machine.I2C and smbus2 could evolve to have a useful intersection (i.e. set of common functions) so that you can use either type of object without caring what it is.