unused parameter warnings in begin()

[bperrybap]

The unused parameter warnings in begin() can be eliminated by using:

void SoftwareWire::begin(uint8_t address)
{
  (void)(address);
  begin();              // ignore the address parameter, the Slave part is not implemented.
}

void SoftwareWire::begin(int address)
{
  (void)(address);
  begin();              // ignore the address parameter, the Slave part is not implemented.
}

Although there is a way to spit out a warning or even an error if these functions are used by using a special declaration in the class. This might be better since this code cause code that is attempting to use slave mode to compile but not work.

You can see my hd44780 library header to see examples of this: https://github.com/duinoWitchery/hd44780/blob/master/hd44780.h

but it will be something like this for an error:

        size_t __attribute__ ((error("begin(address) for slave mode is not supported"))) begin(uint8_t addr);
        size_t __attribute__ ((error("begin(address) for slave mode is not supported"))) begin(int addr);

[Koepel]

Which Arduino IDE version do you use ?

[bperrybap]

I use and test against pretty much every version of IDE from 1.0 to the present 1.8.1 But with respect to this, IDE version isn't what matters, it would be the version of gcc that would be important.

[bperrybap]

On my main development & test machine which is using Linux Mint, I have over 50 different IDEs installed that I can test with. I no longer do any testing on pre Arduino 1.0 since that was mainly there for chipkit MPIDE which is no longer necessary now that chipkit provides a board package for the Arduino IDE. So at this point, I have stopped supporting pre 1.0 IDEs.

[Koepel]

I prefer that the library just runs and ignores the Slave part. The "(void)(address);" could be added, but it is just to fool the compiler. Let's wait for Testato to see what he thinks of it.

[Testato]

added (void)(address);

[bperrybap]

It seems really odd for the library to include the WIRE library slave initialization API but to silently initialize to Master mode when a sketch attempts to use slave mode.

My preference would be to either totally eliminate the slave mode begin(addr) functions or to insert the compiler error with a message to let users know that slave mode is not supported.

IMO, it seem really bad to include API functions that do something totally different than what the official library does and what the documentation says: https://www.arduino.cc/en/Reference/WireBegin

[Koepel]

As being a drop-in-replacement for the Arduino Wire library, they can be useful. However, I tend towards removing the overloading of the two .begin(uint8_t/int address) functions.

[bperrybap]

How can the two begin(addr) functions be useful? They don't actually work. The library only supports master mode and the "Wire" API defines begin(addr) as being used for slave mode. If a sketch attempts to use slave mode by calling begin(addr), the library silently goes into master mode. I think it would be better to let the compile fail so the user knows slave mode isn't supported. And if the error attribute I mentioned in the initial post was used, the library could even terminate the compile with a message that slave mode is not supported.

I guess this issue really should have been about why are begin(addr) functions in the class when slave mode is not implemented? Seems like the best approach would be remove them and replace them with error attributes and print a message that slave mode is not supported. That way the user immediately knows that slave mode is not supported vs just getting a missing method error if the functions were simply removed or silently getting master mode which is probably the worst option - which is what happens now.

[Testato]

the best thing should be implement the i2c slave support by pulsein or something :-)

but for now i agree to print a clear message that the slave is not supported. may you push a PR ?

[bperrybap]

Just now created a fork. I'll submit a PR. I'll need to do some testing/verification to make sure the error attribute it doesn't break on older IDEs.

[Koepel]

I can't do it. As far are I know, the #error or #pragma GCC error are not meant for this. Changing compiler options or making a warning that can be ignored or forcing the use of the parameter into bad code that makes the compiler to trip or using defines or macros or whatever are too ugly to use.

Can we just remove those two overloaded .begin() functions ?

[bperrybap]

I'm not talking about using #error or #warning, or defines or macros or any sort of ugliness to call/use bad code to generate an error.

I'm talking about using the gcc error attribute on a function declaration in the header file.

The error attribute that I noted in the original post of this issue is great for this type of thing. It allows crashing the compiler with a specific error message based on attempting to use a specific function. I use it in my hd44780 library to trap sketches attempting to use println() from the inherited Print class. It allows the hd44780 library to spit out an error that println() from the inherited Print class is not supported should a sketch attempt to use it. The same can be done here to crash the compiler with an error message that slave mode is not supported whenever a sketch attempts to use slave mode by calling begin(addr)

I'll do so testing to make sure there aren't any issues, then I'll submit a PR.

[Testato]

Also the simple #error can be used on arduino core, i used it on other lib.

It is better leave the begin in the lib than completely remove it, for the error message we could use:

error Slave mode is not supported, please help to implement it :-)

[bperrybap]

error is not the answer and doesn't make sense as it cannot be used for this situation.

It also doesn't make sense to leave in the begin(addr) functions.

The only way to have the compiler exit with a specified error message when begin(addr) is called is to use the gcc __attribute__ on the declaration as I showed in the original post of this issue. (go back and take a look at what I originally posted on the github issue on github) The function definitions (the actual code for the two begin(addr) functions) must be removed and #error cannot be used since the error is only emitted based on if the function is called not on defines or conditionals. i.e. calling begin() works but calling begin(addr) will error off with desire error message. This requires support from the compiler and cannot be handled by the pre-processor. To do this, declarations for the unsupported functions - not the definitions, go in the header file but are declared with an error attribute that causes the compiler to emit the desired message and exit with an error should the function is called. We are spending way too much time discussing this. The solution is quite trivial as I showed in the original post of this issue. I'll do some testing to make sure it works on the older compilers shipped with the older IDES and come back with the pull request

[Testato]

Ok, but i remember that #error work. I'll test it, we cannot have right together :-)

[bperrybap]

Yes #error can crash the compiler with an error message, but that isn't good enough. The error should only occur when begin(addr) is called and that is what the error attribute it is for.

[bperrybap]

Note this response is best viewed on the github site.

Ok. So the error attribute works IDE version 1.0.1 which uses avr-gcc version 4.3.2 and with the latest IDE version 1.8.5 which uses avr-gcc version 4.9.2 Code that uses begin() for master mode compiles as expected. Code that attempts to use slave mode by using begin(address) gets a compilation error on the line that calls begin(address) - which I believe is what is desired.

Here is the error message I used: "begin(address) for slave mode is not supported" The reasoning being "begin(address)" is the terminology used on the Arduino Wire library documenation page: https://www.arduino.cc/en/Reference/WireBegin

Both versions of begin(address) are declared with the error attribute in the SoftwareWire.h header

  void __attribute__ ((error("begin(address) for slave mode is not supported"))) begin(uint8_t addr);
  void __attribute__ ((error("begin(address) for slave mode is not supported"))) begin(int addr);

and the definitions for both of them (the code) has been removed from the .ccp file.

Here is the test sketch:

#include <SoftwareWire.h>
SoftwareWire myWire( A4, A5);
void setup()
{
    myWire.begin(10);          // join i2c bus slave
}
void loop() {}

With IDE 1.8.5 you get this output:

In function 'setup',
    inlined from 'main' at /home/bill/Documents/devel/Arduino/ide/arduino-1.8.5/hardware/arduino/avr/cores/arduino/main.cpp:43:9:
begintest:5: error: call to 'begin' declared with attribute error: begin(address) for slave mode is not supported
  myWire.begin(10);          // join i2c bus slave
                  ^
lto-wrapper: /home/bill/Documents/devel/Arduino/ide/arduino-1.8.5/hardware/tools/avr/bin/avr-gcc returned 1 exit status
/home/bill/Documents/devel/Arduino/ide/arduino-1.8.5/hardware/tools/avr/bin/../lib/gcc/avr/4.9.2/../../../../avr/bin/ld: error: lto-wrapper failed
collect2: error: ld returned 1 exit status
Using library SoftwareWire at version 1.4.1 in folder: /home/bill/Documents/devel/Arduino/sketchbook/libraries/SoftwareWire 
exit status 1
call to 'begin' declared with attribute error: begin(address) for slave mode is not supported

And the big orange status bar in the middle of the IDE says: call to 'begin' declared with attribute error: begin(address) for slave mode is not supported

On IDE 1.0.1 the output is:

begintest.cpp: In function ‘void setup()’:
begintest:4: error: call to ‘SoftwareWire::begin’ declared with attribute error: begin(address) for slave mode is not supported

And the big orange status bar in the middle of the IDE says: call to ‘SoftwareWire::begin’ declared with attribute error: begin(address) for slave mode is not supported

While the error messages vary slightly between the IDE versions and compiler versions, the IDE highlights the line with the begin(address) as the line with the error and the error message on the orange bar and the last line of the compile output indicates that salve mode is not supported.

Before I submit a PR I just want to make sure that this is the desired direction and to confirm the exact error message.

[Testato]

I tested the #error directive and you are right, it always elevated also if I do not call begin(address)

So your way is the only one, maybe we can use a simple message text like: I2C/TWI Slave mode is not supported by this library

[bperrybap]

That message could be used. The user would see: call to 'begin' declared with attribute error: I2C/TWI Slave mode is not supported by this library or on old IDEs which use an older version of avr-gcc call to ‘SoftwareWire::begin’ declared with attribute error: I2C/TWI Slave mode is not supported by this library

Maybe instead of "this library" it would be better to mention the actual library? like: I2C/TWI Slave mode is not supported by the SoftwareWire library

[Testato]

Ok for me for the second message. After your PR i will create a Github release so the Arduino library management will send to the user the popup for the update

[bperrybap]

@Testato , the master branch does not work - it does not even compile. You should be testing things before pushing to the master branch. The commit you made on Tuesday: git commit: 3246da79b1c20ca6eadf2ff40b0982a823350ccb broke SoftwareWire.cpp by leaving in a stray #endif

Once you fix that and push it to the master branch to get the code working again, I'll create a branch for this, do the mods, test it, push it to my fork on the branch, and create the PR.

[Testato]

fixed, thanks i also removed duplicated text from .h .cpp compared to Readme.txt

[Koepel]

@bperrybap My apologies, I was testing a few things and forgot the function attribute that this was about. Thanks for you contribution and your effort to increase the quality level.