Skip to content
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

Split software I2C code into Master-only and Master-or-Slave parts, enabling multiple instances of TwoWire in master-only role #7911

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

michkot
Copy link

@michkot michkot commented Mar 8, 2021

Follow-up to the structural change to I2C core support from #6326

What I did is create class hierarchy, where TwiMaster (a subset of the original Twi class in) in "core" does not have any static members, callbacks or IRS, and is exposed in the twi.h header file (in C++ context only).
TwiMasterOrSlave derives from it and adds all the remaining functionality of the original Twi class.
The C interface (declared in twi.h) is rerouted from Twi singleton to TwiMasterOrSlave singleton.

Then, I modified the Wire library in similar manner, but extra measures had to be taken to guarantee compatibility with other Arduino platforms. The default mode of TwoWire class can be switched to master-only via TWOWIRE_MASTER_ONLY define - this implementation does not depend on any static/singleton variables, and also allocates the buffers dynamically (allowing for fine-tuning of necessary buffer size for each I2C interface).

Note: there is already an issue #7894 asking to support multiple I2C instances (with a kinda crude but probably working solution provided already - turning all the static dependencies into 5-tuples so that up-to 5 master/slave I2C interfaces can be used at a time).

@michkot michkot force-pushed the master branch 2 times, most recently from 38a3120 to 68c9c50 Compare March 8, 2021 00:43
- implemented two classes, one support only I2C master role, fully non-static with independent instances, second implementing both master and slave roles, always using the same static TWI singleton class from core
- classes implementing both mods can be always used directly via names TwoWireMaster and TwoWireMasterOrSlave, however that's not a recommand use for portable code/libraries!
- mode (the behaviour or TwoWire class) can be switched from default/compatible master-or-slave to master-only via TWOWIRE_MASTER_ONLY define
- support for optional global Wire instance in both modes
- master-only and master-or-slave modes can used together in some project by splitting code into separate compilation units
@michkot michkot force-pushed the master branch 5 times, most recently from 4de36f4 to f148dcd Compare March 8, 2021 23:44
@michkot
Copy link
Author

michkot commented Mar 8, 2021

There seems to be a bug in the astyle analysis, it forces a weird style change in the initializers list, when you use braces and new keyword is part of the init expression (I changed that code to use parentheses instead).

@devyte devyte self-requested a review March 21, 2021 03:57
eisnerd added a commit to eisnerd/Adafruit_SSD1306 that referenced this pull request Nov 21, 2021
@eisnerd
Copy link

eisnerd commented Nov 21, 2021

I merged over on eisnerd@f41e3a9, though what you probably want is a rebase, which I'm also happy to do. It's just trivial stuff with the switch from ICACHE_RAM_ATTR to IRAM_ATTR.

Not sure what the best way for libraries depending on Wire would be for accepting a TwoWire instance. If they stick with TwoWire, not TwoWireBase, you can't pass TwoWireMaster without building with -DTWOWIRE_MASTER_ONLY=true which for me hosed communication with several i2c devices. Not exactly sure why. They could switch to TwoWireBase if available (just #ifdef?), but this arguably looks surprising to application developers. Could the base class be named TwoWire? This would avoid libraries needing to change. It would break a lot of application code that instantiated TwoWire, but that's presumably less as it would only affect esp8266 and code already using another TwoWire instance in some way.

@michkot
Copy link
Author

michkot commented Nov 27, 2021

I merged over on eisnerd@f41e3a9, though what you probably want is a rebase, which I'm also happy to do. It's just trivial stuff with the switch from ICACHE_RAM_ATTR to IRAM_ATTR.

I will look into merging onto latest;

Not sure what the best way for libraries depending on Wire would be for accepting a TwoWire instance. If they stick with TwoWire, not TwoWireBase, you can't pass TwoWireMaster without building with -DTWOWIRE_MASTER_ONLY=true which for me hosed communication with several i2c devices. Not exactly sure why. They could switch to TwoWireBase if available (just #ifdef?), but this arguably looks surprising to application developers. Could the base class be named TwoWire? This would avoid libraries needing to change. It would break a lot of application code that instantiated TwoWire, but that's presumably less as it would only affect esp8266 and code already using another TwoWire instance in some way.

Hmm.. its some time since I wrote this, I will have to check my own usage. The compatibility problem gave me a huge headache at that time :D

@michkot
Copy link
Author

michkot commented Nov 27, 2021

If they stick with TwoWire, not TwoWireBase, you can't pass TwoWireMaster

Could you maybe just (unsafely) down-cast the wire master-only instance at the place of passing it to library, e.g. static_cast<TwoWire&>(yourWireMasterInstance)?
... hmm now Master and MasterOrSlave are "siblings", not in super/sub class relation, so a double-cast would be needed static_cast<TwoWire*>(static_cast<TwoWireBase*>(&twm)), or the plain old reinterpret_cast<TwoWire*>(&twm) :-) .
I will definitely also look into why I did not make them Base->Master->MasterOrSlave, likely because of the typedef/using hell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants