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

New version breaks pre-existing sketches #128

Open
nseidle opened this issue Feb 26, 2019 · 6 comments
Open

New version breaks pre-existing sketches #128

nseidle opened this issue Feb 26, 2019 · 6 comments

Comments

@nseidle
Copy link

nseidle commented Feb 26, 2019

Hi @amotl - I like your expansion of examples, and moving the dat and clk pins to the begin, but your changes to the constructor break years of pre-existing sketches. I recommend you add back init code to the constructor and possibly overload it to support backwards compatibility. See this issue.

@bogde - I'm happy to do a PR but am hesitant as it's not always clear if or when work will get merged. It looks like you're active these days. Are you interested in this?

@amotl
Copy link
Contributor

amotl commented Feb 26, 2019

Dear @nseidle,

thanks for writing in. I actually thought about doing the facelift in a non-breaking way like some of the people here did by using respective pragma guards. This very detail has already been reflected in the backlog:

[o] Maybe use constructor-based initialization again? It does not necessarily need to starting talking to the hardware yet!

However, if we want to go back to the constructor-as-initializer flavor, we will have to consider some things to make all supported architectures and library maintainers (is it us, actually?) happy together. While I will be very happy to support you in regaining backward compatibility for this library (thanks again, I hear you regarding the issue sparkfun/OpenScale#29!), I also would like to share some thoughts we learned throughout the process of #123 with both of you.

In order to do so, I will followup up with a more detailed comment reflecting my thoughts. Thanks for listening already.

Cheers,
Andreas.

@amotl
Copy link
Contributor

amotl commented Feb 27, 2019

Introduction

We just became aware that while this library was directly referenced by OpenScale Applications and Hookup Guide » Upgrading the Firmware, the corresponding OpenScale.ino would obviously not be in sync with eventual changes coming from upstream.

By introducing the breaking changes, we haven't exactly been aware of the impact this would have to OpenScale users and want to apologize for the hassle @MIKMAI and you had when being hit with this issue out of the blue after @bogde merged our changes.

Background

While trying to solve different issues people were having with this library, we came to the conclusion to break the interface intentionally and that updating the canonical examples and documenting the fact by putting a deprecation warning into the README would be enough. From how this has led to problems with downstream users of this library - in this case all users of OpenScale - we are now learning that we should be more careful with things like that in the future.

Thoughts

Personally, I believe every upstream software artefact should provide stable release packages of its own in order to streamline consumption from downstream. Versioned dependencies you can actually depend on are not really a bad thing at all.

After learning bits and pieces of the Arduino library ecosystem when there was no package repository and corresponding library manager out there, we are now happy to see the PlatformIO Ecosystem will probably be here to stay and so we all should just start using it to make the magic happen, shouldn't we?

As coming from a Python background, we can tell you the Python Package Index is actually one of the best pieces of Python, as the Comprehensive Perl Archive Network is for Perl.

Proposed solution

There are currently two ways to provide stable download links referencing release packages of the HX711 library:

  • As the library already is available on PlatformIO, there's already a direct download link [1].
  • When tagging the GitHub repository, GitHub itself will already consider this action as the intention to declare a (stable) release and provide .zip as well as .tar.gz archives of the respective revisions on the releases page of the repository. This would yield direct download links like [2].

Contents from both archives should yield code which should have been compatible with programs, wrapper libraries and firmwares like OpenScale out there.

[1] http://dl.platformio.org/libraries/archives/11/2527.tar.gz?filename=HX711_0.1_1100
[2] https://github.com/bogde/HX711/archive/v0.1.tar.gz

Outlook

  • We should document the fact that release packages are available through different channels prominently on top of the README page as well as on top of the HX711.{h,cpp} files.
  • We should actually start tagging releases on the GitHub repository. May we humbly ask @bogde about this? (Remark: We already wanted to open a respective issue ticket, but haven't been able to find additional time for that).

@amotl
Copy link
Contributor

amotl commented Feb 27, 2019

Regarding the breaking change itself

However, if we want to go back to the constructor-as-initializer flavor, we will have to consider some things to make the code run happily on all supported architectures.

We had to decouple the object constructor from calling the begin() method implicitly to make the thing work on ESP8266 and ESP32 chips, as with both architectures, Arduino code is not running on the bare metal.

So, when code running on one of these architectures starts talking to GPIO or other hardware interfaces on object initialization time already, it is way too early and things will probably go south.

Rationale

In order to account for that while not obfuscating the code too much, we had to decide whether to break on compilation time or on runtime and obviously decided for the earlier as the latter would hit users way harder when everything compiles as expected and then would just stop working because nobody did ever call begin() before, which is now obligatory, see earlier version of OpenScale.ino.

Regaining backward compatibility

There actually would be a way to make the library interface backward compatible again. Let's remember we have to a) change the signature of the constructor back to its original shape and b) make the library work even without calling begin().

These steps should be sufficient:

  • Let the constructor obtain the pin definition again.
  • Let the library implicitly call begin() once when being called on read().

Thoughts

However, is it worth it? If one of you would find regaining backward compatibility important, we will be happy to update the code correspondingly.

@bogde
Copy link
Owner

bogde commented Mar 1, 2019

I too apologize for the hassle @nseidle and @MIKMAI had. I was not aware merging the changes would have such an unwanted impact. I do think that these changes add major improvements to the code and solve issues that annoyed the users for too long though. I want to thank @amotl and all the other contributors again for the time and effort they put into this recently.

I just created two releases, one for the code that we had before the spring cleaning and just after changing the license to MIT (0.3.0) and the other one for the current code that breaks compatibility with existing sketches (0.7.1).

I would be happy to pull any changes but, same as Andreas, I wonder if regaining backward compatibility is worth the effort. I'm not saying or implying it isn't, I'm just humbly asking for your opinion.

@amotl
Copy link
Contributor

amotl commented Mar 1, 2019

Thank you so much for cutting a new release, @bodge! Let's check what the community will say about this. I will still be around for assistance, please ping me as needed.

@nseidle
Copy link
Author

nseidle commented Mar 1, 2019

No worries folks. Not breaking things can be hard to avoid.

I think I've fixed our firmware and our hookup guide example code so future users should be ok.

I'm ok closing this issue. But I have some recommendations for library encapsulation that I'll start on another issue.

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