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.

Sign up for GitHub

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

Jump to bottom

Added option to use SBWIRE to avoid well-known I2C hangup problems with Wire lib #160

Closed
wants to merge 5 commits into from
Closed

Added option to use SBWIRE to avoid well-known I2C hangup problems with Wire lib #160

wants to merge 5 commits into from

Conversation

paynterf
Copy link

The Arduino Wire library has a well-known hangup problem where blocking read & write calls never return. SBWire adds timeouts to all potentially blocking calls. This pull request simply adds the option to switch from the Wire library to SBWIRE.

Note that I submitted this request Feb 17, 2019 but it never got merged.

@paynterf
Copy link
Author

Not quite sure how/why the clang check failed, but the modified files compile fine for me. If there is something I can do to fix this problem, please let me know

@edgar-bonet
Copy link
Contributor

Adafruit is now enforcing strict code formatting rules through the clang-format utility. If you click on the failed build, and then on “clang” to expand the details, you will see the changes that clang-format requires for the test to pass. It's just about keeping the source lines within 80 characters and avoiding consecutive empty lines:

--- ./RTClib.h	(original)
+++ ./RTClib.h	(reformatted)
@@ -25,8 +25,8 @@
 #include <Arduino.h>
 class TimeSpan;
 
-//#define USE_SBWIRE //uncomment this to use SBWire, which fixes Wire library lockup problems
-
+//#define USE_SBWIRE //uncomment this to use SBWire, which fixes Wire library
+//lockup problems
 
 /** Registers */
 #define PCF8523_ADDRESS 0x68       ///< I2C address for PCF8523
--- ./RTClib.cpp	(original)
+++ ./RTClib.cpp	(reformatted)
@@ -46,7 +46,8 @@
 #ifdef __AVR_ATtiny85__
 #include <TinyWireM.h>
 #define Wire TinyWireM
-#elif defined USE_SBWIRE //Uncomment this definition in RTClib.h to avoid Wire library lockups
+#elif defined USE_SBWIRE // Uncomment this definition in RTClib.h to avoid Wire
+                         // library lockups
 #include <SBWire.h>
 #else
 #include <Wire.h>

@paynterf
Copy link
Author

OK, I committed changes to satisfy clang - how do I re-run the tests? Does this happen automatically?

@paynterf
Copy link
Author

another clang nitfix

@paynterf
Copy link
Author

two more clang nitfixes

@siddacious
Copy link
Contributor

@paynterf Yes, the tests are re-run automatically when you push to the branch you are asking to be pulled.

It sounds like you're attempting to make the changes manually. I'd suggest letting clang-format do the work for you. You should be able to find it thought your system's package manager, or by compiling it yourself.

clang-format -i <filename>

@siddacious siddacious mentioned this pull request Apr 26, 2020
Closed
@edgar-bonet
Copy link
Contributor

Having myself been bitten by this issue of Arduino Wire (which I then “fixed” with the watchdog), I fully appreciate the value of giving users the option to use a better I2C library. I am not convinced, however, by the approach taken by this pull request, as it requires the user to alter the source code of the library. This brings a couple of issues:

  1. This is not something users are normally supposed to do, and most may not know how to do it unless they are given clear instructions.
  2. Those modifications will be undone when upgrading the library.

Regarding point 1, it is admittedly not very hard to document the procedure and ask the user to “uncomment #define USE_SBWIRE at the top of RTClib.h”. It is, however, just as easy to ask him to “replace #include <Wire.h> by #include <SBWire.h> at the top of RTClib.cpp”.

I wonder whether there is a better option that doesn't require the user to modify the library sources. I guess the nicest solution would look like a template class with a default template argument that gives the user the option to instantiate their RTC as either

RTC_DS1307 rtc;  // based on classic Wire, for API stability

or

RTC_DS1307<SomeAlternativeWireImplementation> rtc;

for those who want the better option.

I am not sure I would be able to implement that though, as I am not very fluent with C++ templates. Mostly I fear that the presence of many static methods in the RTC classes may preclude this approach. Am I right in assuming that making those methods non-static would be an unacceptable API change?

@jadonmmiller
Copy link
Contributor

jadonmmiller commented Apr 26, 2020 •
edited

@edgar-bonet I've not heard of the i2c hanging before, but I think that's the source of a really frusterating issue I'm having right now. I have a Ds3231 and oled connected via i2c on an stm32 (I first noticed the issue on a pro mini) and the code seems to stop at the endTransmission in the rtc begin function.

Does that sound familiar? If that is my problem, I'd be in full support of adding capability for an alternate wire library, though I agree with your points about the implementation.

I haven't really thought this through, but could we implement the OP's .cpp modifications, and put the #define to swap wire libraries in the user's code, above the #include <RTClib.h>?

It's just a quick thought I had. If the new wire library is a drop-in replacement for the old, it should somehow be easy to implement in a clean way.

@edgar-bonet
Copy link
Contributor

@jadonmmiller wrote:

could we implement the OP's .cpp modifications, and put the #define to swap wire libraries in the user's code, above the #include <RTClib.h>?

That won't work. The problem is that the library and the user code are compiled separately. When compiling RTClib.cpp, the compiler does not look at the user code.

That is, unless the whole library is turned into a header-only library, or at least the bits accessing the hardware are all moved to the header file. Note that libraries based on templates tend to be header-only, or at lest header-heavy.

@jadonmmiller
Copy link
Contributor

@edgar-bonet Okay, thanks for your input! I'm by no means a good C++ programmer and hadn't considered that. There's gotta be some way to easily do it without the user modifying the library! 😃

Thanks!
Jadon

@paynterf
Copy link
Author

paynterf commented Apr 26, 2020 via email

@paynterf
Copy link
Author

paynterf commented Apr 26, 2020 via email

@ladyada
Copy link
Member

ladyada commented Apr 26, 2020

hi looks like there is a PR open - you could politely add any notes or recommendations there
arduino/Arduino#2489

@ladyada
Copy link
Member

ladyada commented Apr 26, 2020

closing this PR - it is now off topic, please fork if you need a different version of Wire

@ladyada ladyada closed this Apr 26, 2020
@adafruit adafruit locked as off-topic and limited conversation to collaborators Apr 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@paynterf @edgar-bonet @siddacious @jadonmmiller @ladyada