bperrybap
commented
1 year ago I appreciate the effort to add new functionality but this pull request will not be accepted.
Perhaps you are not aware that Wire.begin() can be called multiple times and if it is called as Wire.begin(sdaPin, sclPin) and then later as Wire.begin() that the pins configured will still be used vs the defaults. Also there is also the Wire.pins(sdaPin, sclPin) call that can be used to set the pins. https://github.com/esp8266/Arduino/blob/master/libraries/Wire/Wire.cpp
There are three main reasons why the pull request will not be accepted:
- it is adding h/w specific code to a layer that is h/w agnostic
- "as is" , it is an undocumented / secret functionality for certain i/o classes on a limited number of core platforms
- The net effect of this addition is that it doesn't add any new functionality that doesn't already exist (users can already set these pins)
Here is the detailed explanation as to why.
The hd44780 library is a multi class layered library. The layers are intended to intentionally separate out the workings of the library API and hd44780 functionality from the control of the h/w that communicates to the LCD h/w.
The hd44780 class code only handles the API, hd44780 instructions, and hd44780 timing. It knows nothing about h/w used to communicate to the LCD h/w. This is very intentional and will not be changed. This allows any / all code in the hd44780 layer to work on all i/o classes which provides a clean an consistent API interface across all the i/o classes.
Putting any h/w specific interface knowledge and code into the hdd44780 class layer violates that layer separation. Also, EVERY API function in the hd44780 library returns status to indicate whether the API function succeeded. Any extension to the hd44780 APIs would not only have to be generic enough to work on all i/o classes but must also return status as to whether it succeeded. i.e. an API function cannot be a void function.
In this specific case, the code is used to configure the i2c pins used by the ESP core. The ESP cores are pretty unique in this respect since they are pretty much the only cores that allow this since i2c is implemented in s/w on the ESP parts vs having dedicated h/w and specific pins for i2c like what is in the AVR, ARM, and PIC microcontrollers.
Also, there is already a way to configure these pins without this code.
In stepping back a bit to see how this affects user sketch code, this specific pull request doesn't really add any actual benefit to the user.
I know that sounds harsh, but consider what a user must do to configure the ic2 pins.
He must modify his mainline sketch code to set these pins by doing something before he calls lcd.begin()
And once something has to be done for a specific environment, it doesn't much matter whether the code calls foo() or call bar() to do it.
In this case, the user would have to insert a function call before calling begin()
without this pull request code the user would call
Wire.begin(sdaPin, sclPin):
or
Wire.pins(sdaPin, sclPin);
And after this modification the user would call
lcd.setCustomPins(sdaPin, sclPin);
The net effect of this pull request addition is that it doesn't really add anything. It just changes the specific call being done. i.e. a user can already set custom pins in the ESP environment This addition is no better than what exists today, it is just slightly different.
Any time changes are being made or proposed, there is one important step that should always be done and it seems that too often these days many s/w developers no longer do it or even consider it.
That is, when looking at proposed changes and their full ramifications, are they really making things better? i.e. is the net result better, just different, or actually worse?
In this case from the user of the library point of view it is no better, no worse than what they already have today. Just different. i.e. they call bar() instead of foo() It does not affect any existing code so there is no backward compatibility considerations. From a library code point of view it is worse, it is inserting h/w specific code into a h/w agnostic layer and it also means modifying every i2c i/o class to support it. And then there is also testing and documentation to do a modification like this. i.e. if a new API function is added is must be documented so users know it exists. It also brings along potential long term support and backward compatibility issues.
So in the end, for several reasons, I can not accept this modification.
alx3dev
Hello, and thank you for a great library. I've added a function to set custom I2C pins, different from hardware ones. I did it so I can use it with some old Esp-01, but could be useful for other devices too. Feel free to merge it, or not...