Open matthijskooijman opened 3 years ago
Sounds good; this is partly because all the interfaces in sensirion_hw_i2c_implementation.cpp
use generic - i.e. not platform specific - APIs, which is why we had the sensirion_i2c_select_bus()
call in there with an index; changing the interface to an Arduino SDK one would break compilation on other platforms.
So this will require a little bit more thought on our end, or potentially just foregoing compatibility with embedded-common
in the interest of better support of Arduino platform needs
changing the interface to an Arduino SDK one would break compilation on other platforms.
Yeah, so maybe adding a function (i.e. sensirion_i2c_select_wire(...)
or so) could work? And then just ignoring the existing sensirion_i2c_select_bus()
?
Even though the existing index-based API isn't necessarily bad, I'm afraid it will be hard to implement fully portably Arduino, since there is no generic index -> Wire object API, or a standardized and documented way to figure out how many wire interfaces there are.
Looking more closely, it does seem that there is a WIRE_INTERFACES_COUNT
macro that is at least used by the Arduino sam
, Arduino samd
, Adafruit samd
, stm32l0
and stm32l4
cores, so that could be a reasonably standard way to do this maybe? It seems that Teensy implements WIRE_IMPLEMENT_WIREX
macros instead. So an alternative could be maybe:
int16_t sensirion_i2c_select_bus(uint8_t bus_idx) {
switch (bus_idx) {
case 0:
SensirionWire = &Wire;
return 0;
#if defined WIRE_IMPLEMENT_WIRE1 || WIRE_INTERFACES_COUNT > 1
case 1:
SensirionWire = &Wire1;
return 0;
#endif
#if defined WIRE_IMPLEMENT_WIRE2 || WIRE_INTERFACES_COUNT > 2
case 1:
SensirionWire = &Wire2;
return 0;
#endif
#if defined WIRE_IMPLEMENT_WIRE3 || WIRE_INTERFACES_COUNT > 3
case 1:
SensirionWire = &Wire3;
return 0;
#endif
default:
return STATUS_FAIL;
}
}
(as an alternative implementation for sensirion_i2c_select_bus
, the other changes suggested above are still needed)
The generic usage we thought of is quite similar but shifts more logic in to the i2c read and write functions based on the current context. A quick recap: The HAL implementation is aware of all three implementations: sensirion_i2c_read
, sensirion_i2c_write
and sensirion_i2c_select_bus
- the latter being the trigger to switching the context (since we don't pass context objects, those have to be maintained separate, e.g. as globals.) Thus, we modify the behavior of sensirion_i2c_read
depending on the context, which is triggered by the index. E.g. the read function would then look like this switch (index) { case 1: Wire1.read(); // ...
Ah, that would probably be even better, since then the code will even work when Wire
and Wire1
have different types somehow.
+1
The cleanest way forward would be to port this to https://github.com/Sensirion/arduino-core/ since that allows selecting Wire objects already.
In the interest of time, maybe we could just modify sensiron_i2c_init()
to take a reference to an initialized Wire object... @psachs, thoughts?
Any further thoughts on this issue? Would be good if we could switch back to an unmodified arduino-sps library for our project.
On some boards, such as STM32 boards (and probably SAMD boards too), there are multiple I²C controllers available, typically named
Wire
,Wire1
, etc.Currently, this library always uses
Wire
hardcoded, but it would be convenient if a secondary bus could also be used.There is already a
sensirion_i2c_select_bus(uint8_t idx)
function declared, but it is never defined anywhere. Also, since I'm not sure whether there is a standard way to detect how much Wire objects are defined, this API might be hard to implement on Arduino. Therefore, I'd suggest implementing a selection function that actually passes theWire
,Wire1
, etc objects as a reference. The type of these seems to beTwoWire
on most cores, but just in case cores use other names, I'd suggest usingdecltype(Wire)
instead.So, that would be something like this (untested, should probably be split between .h and .cpp files as well, but just to get the general idea):
And then replace all other
Wire.
withSensirionWire->
too.This allows a sketch to do e.g.:
It can even switch between multiple buses at runtime, as long as it does call
sensiron_i2c_init()
once for each bus beforehand.