Skip to content

revert remove of defines #6555

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
wants to merge 1 commit into from
Closed

Conversation

Jason2866
Copy link
Collaborator

@Jason2866 Jason2866 commented Apr 9, 2022

since it is a uneeded breaking change. For example this library do not compile without https://github.com/earlephilhower/ESP8266Audio

Partially revert of #6527 @me-no-dev

By completing this PR sufficiently, you help us to improve the quality of Release Notes

Checklist

  1. Please provide specific title of the PR describing the change, including the component name (eg. „Update of Documentation link on Readme.md“)
  2. Please provide related links (eg. Issue, other Project, submodule PR..)
  3. Please check Contributing guide

This entire section above can be deleted if all items are checked.


Summary

Please describe your proposed PR and what it contains.

Impact

Please describe impact of your PR and it's function.

Related links

Please provide links to related issue, PRs etc.

since it is a uneeded braking change. For example this library do not compile without https://github.com/earlephilhower/ESP8266Audio
@github-actions
Copy link
Contributor

github-actions bot commented Apr 9, 2022

Unit Test Results

0 files  0 suites   0s ⏱️
0 tests 0 ✔️ 0 💤 0

Results for commit 3cdaed5.

@dok-net
Copy link
Contributor

dok-net commented Apr 9, 2022

Please explain how removing #defines that in my understanding are now without function long after the original breaking change is itself a breaking change, or how it's bad if code that might need to adapt to that fails to compile?
This is for now a gut-feeling, I haven't checked in detail what the lib you mentioned expects the effect of employing any of the now-defunct #defines to be :-)

@Jason2866
Copy link
Collaborator Author

Jason2866 commented Apr 9, 2022

Since variable SPECIAL #define SPECIAL 0xF0 is used in the code
And as i wrote why change something that does zero here and brake existing code?

lib/lib_audio/ESP8266Audio/src/spiram-fast.h: In member function 'void ESP8266SPIRAM::begin(int, int)':
lib/lib_audio/ESP8266Audio/src/spiram-fast.h:335:26: error: 'SPECIAL' was not declared in this scope
             pinMode(sck, SPECIAL);
                          ^~~~~~~
lib/lib_audio/ESP8266Audio/src/spiram-fast.h:335:26: note: suggested alternative: 'SERIAL'
             pinMode(sck, SPECIAL);
                          ^~~~~~~
                          SERIAL

@dok-net
Copy link
Contributor

dok-net commented Apr 9, 2022

@Jason2866 pinmode(..., SPECIAL); had some semantics that are no longer there. Unless you can prove that the state of the ESP32 that previous users of those defines require is given, and that using pinmodewith SPECIAL doesn't actually break anything now, shouldn't code doing so rather raise an error now that just fail at runtime?

@Jason2866
Copy link
Collaborator Author

Jason2866 commented Apr 9, 2022

What wrong with pinMode(sck, 0xF0) ?
To be clear using a PHY8720 with and without your change does work.
So the defined Functions are still working

@dok-net
Copy link
Contributor

dok-net commented Apr 9, 2022

@earlephilhower could you please check your library? Thanks.

@Jason2866
Copy link
Collaborator Author

Jason2866 commented Apr 9, 2022

@dok-net The ESP8266Audio lib was just an example. I do not understand your reaction with the thumb. The fact that a PHY8720 is working without the patch for my board?
I do not say that your PR is not usefull and solves probably issues for a lot of people.

@dok-net
Copy link
Contributor

dok-net commented Apr 9, 2022

@Jason2866 I disgree with your conclusions, like, I believe clean code requires the removal of technical debt when you find it. I could not tell if you were referring to the patch as a whole or just the SPECIAL define. In my opinion, it's up to the library owners to adapt their code to breaking changes in Arduino Core for ESP32, I think your report would be better off there. We should stop debating this point now. Unless you research and find that SPECIAL both still has any effect, and that it's not an ill effect, either ;-)

@atanisoft
Copy link
Collaborator

atanisoft commented Apr 9, 2022

Since variable SPECIAL #define SPECIAL 0xF0 is used in the code

If you look into the file further the usages are mostly ifdef wrapped for ESP8266 only. They don't appear to be necessary or valid use cases in this usage anyway. I'd suggest file an issue over there and have them fix their broken usage.

@Jason2866
Copy link
Collaborator Author

@dok-net In a perfect world i would agree with you.

@me-no-dev
Copy link
Member

@Jason2866 I would rather not have things that do not work. pinMode(pin, SPECIALx) no longer works and the library needs to use direct IDF if needs those modes

@Jason2866
Copy link
Collaborator Author

@me-no-dev Thx, for pointing out. Still wondering why the PHY8720 code works with and without this PR.

@Jason2866 Jason2866 closed this Apr 11, 2022
@Jason2866 Jason2866 deleted the patch-2 branch April 11, 2022 10:33
@me-no-dev
Copy link
Member

@Jason2866 probably because pins are also set internally in the ETH driver in ESP-IDF

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

Successfully merging this pull request may close these issues.

4 participants