-
Notifications
You must be signed in to change notification settings - Fork 542
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
Comments
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:
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, |
IntroductionWe 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. BackgroundWhile 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. ThoughtsPersonally, 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 solutionThere are currently two ways to provide stable download links referencing release packages of the HX711 library:
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 Outlook
|
Regarding the breaking change itself
We had to decouple the object constructor from calling the 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. RationaleIn 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 Regaining backward compatibilityThere 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 These steps should be sufficient:
ThoughtsHowever, is it worth it? If one of you would find regaining backward compatibility important, we will be happy to update the code correspondingly. |
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. |
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. |
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. |
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?
The text was updated successfully, but these errors were encountered: