Skip to content

Update IDF to aaf1239 breaks Wire library endTransmission() #1725

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

Closed
bperrybap opened this issue Aug 4, 2018 · 6 comments
Closed

Update IDF to aaf1239 breaks Wire library endTransmission() #1725

bperrybap opened this issue Aug 4, 2018 · 6 comments

Comments

@bperrybap
Copy link
Contributor

The Update IDF to aaf1239 here: a59eafb#diff-f1344d8f8dd31c4411770e150a4425fc
Includes some updates to the Wire library. One of the updates is a new endTransmission(bool) function.
While it would be nice to have this function use a bool instead of a uint8_t , adding it along side of the existing uint8_t functions breaks existing code that calls the existing endTransmission(uint8_t) function with 0 or 1.
i.e. existing applications or libraries that make calls to endTransmission(0) and endTransmission(1) will break since it is ambiguous which endTransmission() function should be called the bool or the uint8_t version.

I have not seen any other Arduino core implement a endTransmission(bool) function.
In order to maintain compatibility with existing 3rd party i2c applications and 3rd party i2c libraries the endTransmission(bool) function will need to be removed.

@bperrybap
Copy link
Contributor Author

While 3rd party s/w can work around this issue, since it is an issue that has been created in the ESP32 Wire code, the proper place to fix it is in ESP32 vs all the 3rd party s/w.

@stickbreaker
Copy link
Contributor

Official Arduino Wire.endTransmission()https://www.arduino.cc/en/Reference/WireEndTransmission

@bperrybap
Copy link
Contributor Author

While that reference page uses the words "boolean" and "true" the AVR code that Arduino.cc supplies does not use a C/C++ bool type for endTransmission()
https://github.com/arduino/ArduinoCore-avr/blob/master/libraries/Wire/src/Wire.h
However Arduino.cc do seem to use a bool type for SAMD/DUE:
https://github.com/arduino/ArduinoCore-samd/blob/master/libraries/Wire/Wire.h
So even they have issues - but they don't have compilation issues when using endTransmission(0) or endTransmission(1) like ESP32 does.

While DUE uses a bool type, pretty much all other cores use uint8_t for the type rather than bool.

What may also create some issues/confusion is that "boolean" is not a C/C++ type it is an Arduino proprietary type.
In the past "boolean" was a uint8_t but in recent IDEs it has been retyped to be an actual bool.

But the bottom line is that the ESP32 core Wire library now includes both endTransmission(bool) and endTransmission(uint8_t)
I've not seen any other core or 3rd party i2c library have both of these.
And it is the inclusion of both functions that creates issues for existing code since endTransmission(0) and endTransmission(1) no longer compiles when using the ESP32 core.
This is a new issue because of the recent update that added endTransmission(bool) but left endTransmission(uint8_t).

As of right now, I've not seen any other core or library "Wire" API implementation that fails to compile endTransmission(1) and endTransmisssion(0) other than the ESP32 core.
So I'll repeat that while 3rd party code that uses endTransmission(0) and endTransmission(1) could be updated to work around this issue in the ESP32 core Wire library, since it is a new issue in the ESP32 core Wire library, IMO it really should be fixed there.

One or the other of the two functions could be removed to ensure that endTransmission(0) and endTransmission(1) continue to work as they have in the past and continues to work in other cores like
Arduino AVR, Arduino SAMD, Arduino ARM, Teensy, Teensy3, chipkit, esp8266, STM32, Intel Edison etc...,
and in other i2c libraries like: TinyWireM, SoftwareWire,

@bperrybap
Copy link
Contributor Author

bperrybap commented Sep 22, 2018

Just to be clear, I'm not asking to change endTransmission(stop) to use a uint8_t parameter, I'm saying that
endTransmission(0) and endTransmission(1) do not work in the ESP32 core but does work in every other Arduino core.

This is because the function was overloaded to take either a uinit8_t or a bool
One of those needs to be removed as that is what creates the ambiguity and breaks endTransmission(0) and endTransmission(1)
If the bool type is preferred, then the other uint8_t version could be removed.

@bperrybap
Copy link
Contributor Author

The solution to this is to simply remove the uint8_t TwoWire::endTransmission(uint8_t sendStop)
version of the function.
There is absolutely no need for it and including it breaks all existing code that uses endTransmission(0) and endTransmission(1)

The point of having the uint8_t version of the function was to support endTransmission(0) and endTransmission(1) yet by having it and the bool version those calls break.

By having only the bool version of the function you get the best of all words.
Since 0 equates to false and 1 equates to true, when you have only the bool version of the function users can use:

endTransmission(false) 
endTransmission(0)
entTransmission(true)
endTransmission(1)

This allows everything to work. Old/existing code that uses 0 and 1 and new code that uses false and true.
Either can work if you only define the bool version of the function.

Including both versions of the function doesn't solve anything and simply creates several problems.

  1. It breaks existing code that uses endTransmission(0) and endTransmission(1) (which works on all other cores)
  2. It makes it impossible to create portable code that works on all Arduino cores.
  3. It requires using ifdefs specifically for the ESP32 core which is silly.

Currently, the way for code to be compatible with all cores, some of which use uint8_t and some that use bool, is to use endTransmission(0) and endTransmission(1)
However, that breaks on the ESP32 core so ifdefs have to use for ESP32.

I'll provide a PR to fix this.

@bperrybap
Copy link
Contributor Author

PR created: #1888

me-no-dev pushed a commit that referenced this issue Nov 19, 2018
* removed uint8_t Wire.endTransmission(uint8_t sendStop)

Having both endTransmission(bool) and endTransmission(uint8_t) creates problems.
There is no need for endTransmission(uint8_t)
endTransmission(1) and endTransmission(0) works with endTransmission(bool).
Removing endTransmission(uint8_t) allows the ESP32 code to be compatible with
all the other Arduino cores by allowing endTransmission(1) and endTranmission(0)
to work as it does on all the other cores.

* Wire library version bump for endTransmission() update
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

No branches or pull requests

3 participants