Avoid compiler warnings about aux3...aux9

[esspe2]

Variables aux3 ... aux9 needed to be initialized and used, or the compiler emits warnings.

A negative value has been set initially, and dummy actions avoid the warning. If in doubt, INPUT_PULLUP was used so that pins don't float and don't fire any interrupts for no reason.

This version doesn't use functions as workaround against the warnings, so assignations have been put back in setupAxes(), hopefully their intended place originally.

[MaslowCommunityGardenRobot]

Congratulations on the pull request @esspe2

Now we need to decide as a community if we want to integrate these changes. You should vote by giving this comment a thumbs up or a thumbs down. Votes are counted in 48 hours. Ties will not be merged.

I'm just a robot, but I love to see people contributing so I'm going vote thumbs up (but my vote won't count...)!

[blurfl]

I'm not seeing those compiler warnings using the Arduino IDE 1.8.9, even with 'All' compiler warnings and 'verbose' output enabled. I don't see them in PlatformIO either. How could one find them to test the patch? I think I see, this is an improvement on the previous 'compiler work-around', pulled back into System::setupAxes() instead of happening separate functions. That does get all the setup done in one place 👍. I'd suggest adding the definitions of aux7..aux9 to each of the (pcbVersion == 0), ...1, ...2 sections to avoid the logic stub

}else{ // default values to keep compiler from complaining

on the end of the (pcbVersion == 3) section. Subsequent board versions would make that test messier.

Assigning them negative numbers to flag unused values is a good approach. This should should work for users who have made local changes to the AUX assignments for their own setups - their values won't be overwritten.

I think that defining the 'unused' AUX pins as INPUT would be safer than INPUT_PULLUP, though. We don't know what might be connected to those pins; they are initialized as INPUT by the Arduino firmware initialization routine which leaves them at 0 volts. The INPUT_PULLUP option puts IOREF voltage (3v3 or 5v) on the pin and might turn something on unintentionally.

[esspe2]

Strange, I have used the 1.8.2 version and didn't think that could make such difference. Will switch shortly and correct the mistake of the aux5 line.

[esspe2]

I am still able to see the warnings in 1.8.9... Here is the same gcc line as launched by the IDE, I see -Wall -Wextra :

$ rm /tmp/arduino_build_917408/sketch/System.cpp.o
$ ~/.arduino15/packages/arduino/tools/avr-gcc/5.4.0-atmel3.6.1-arduino2/bin/avr-g++ \
>        -c -g -Os -Wall -Wextra -std=gnu++11 -fpermissive -fno-exceptions \
>        -ffunction-sections -fdata-sections -fno-threadsafe-statics \
>        -Wno-error=narrowing -MMD -flto -mmcu=atmega2560 -DF_CPU=16000000L \
>        -DARDUINO=10809 -DARDUINO_AVR_MEGA2560 -DARDUINO_ARCH_AVR \
>        -I~/arduino-1.8.9/hardware/arduino/avr/cores/arduino \
>        -I~/arduino-1.8.9/hardware/arduino/avr/variants/mega \
>        -I~/arduino-1.8.9/libraries/Servo/src \
>        -I~/arduino-1.8.9/hardware/arduino/avr/libraries/EEPROM/src \
>        /tmp/arduino_build_917408/sketch/System.cpp \
>        -o /tmp/arduino_build_917408/sketch/System.cpp.o
/tmp/arduino_build_917408/sketch/System.cpp:254:43: warning: unused parameter 'aux3' [-Wunused-parameter]
 void configAuxLow(int aux1, int aux2, int aux3, int aux4, int aux5, int aux6) {
                                           ^
/tmp/arduino_build_917408/sketch/System.cpp:254:63: warning: unused parameter 'aux5' [-Wunused-parameter]
 void configAuxLow(int aux1, int aux2, int aux3, int aux4, int aux5, int aux6) {
                                                               ^
/tmp/arduino_build_917408/sketch/System.cpp:254:73: warning: unused parameter 'aux6' [-Wunused-parameter]
 void configAuxLow(int aux1, int aux2, int aux3, int aux4, int aux5, int aux6) {
                                                                         ^
/tmp/arduino_build_917408/sketch/System.cpp:262:24: warning: unused parameter 'aux7' [-Wunused-parameter]
 void configAuxHigh(int aux7, int aux8, int aux9) {
                        ^
/tmp/arduino_build_917408/sketch/System.cpp:262:34: warning: unused parameter 'aux8' [-Wunused-parameter]
 void configAuxHigh(int aux7, int aux8, int aux9) {
                                  ^
/tmp/arduino_build_917408/sketch/System.cpp:262:44: warning: unused parameter 'aux9' [-Wunused-parameter]
 void configAuxHigh(int aux7, int aux8, int aux9) {
                                            ^
$ 

Or maybe it's because my preferences.txt file contains these lines :

build.verbose=true
compiler.warning_level=all

At the end, that also could be from the ~/.arduino15/packages/arduino/hardware/megaavr/1.6.26/platform.txt file:

compiler.warning_flags=-w
compiler.warning_flags.none=-w
compiler.warning_flags.default=
compiler.warning_flags.more=-Wall
compiler.warning_flags.all=-Wall -Wextra

[esspe2]

Cannot see better solution than this bunch of -1 's assignations for now;

I'd suggest adding the definitions of aux7..aux9 to each of the (pcbVersion == 0), ...1, ...2 sections to avoid the logic stub

But can you guarantee that all cases are taken into account without a last else ? I fear the compiler is too dumb for that: you can see warnings related to aux3, aux5, ... too.

[blurfl]

But can you guarantee that all cases are taken into account without a last else ?

That last else changes the value of all the auxX variables for all except whatever the is last (pcvVersion == ...). Before the patch, one could use those variables to reference gpio pins as defined by a specific board.

Cannot see better solution than this bunch of -1 's assignations for now;

What do you think about this approach

// Assign AUX pins to extern variables used by functions like Spindle and Probe
void configAuxLow(int aux1, int aux2, int aux3, int aux4, int aux5, int aux6) {
  SpindlePowerControlPin = aux1;   // output for controlling spindle power
  ProbePin = aux4;                 // use this input for zeroing zAxis with G38.2 gcode
  LaserPowerPin = aux2;            // output for controlling a laser diode
  pinMode(LaserPowerPin, OUTPUT);
  digitalWrite(LaserPowerPin, LOW);
  int unused;
  unused = aux3;
  unused = aux5;
  unused = aux6;
}

void configAuxHigh(int aux7, int aux8, int aux9) {
  int unused;
  unused = aux7;
  unused = aux8;
  unused = aux9;
}

That satisfies the compiler warnings listed above, though it generates an error that 'unused' is .... unused.

[esspe2]

That last else changes the value of all the auxX variables

I don't think so: from my point of view, each auxX receive only one value, precisely in one of the if's or in the last else. If it is set in the if's, the else won't do anything. There is always only one assignment for all variables (unless I'm mistaken).

Beware, this PR relates to two warnings: uninitialized variables and unused variables.

For the uninitialized variables, I think the compiler won't be happy unless we give him the else statement, or some initial value like int aux1=-1; and s.o. at line 96. Question of taste.

If the famous else is executed, we know that no board was recognized, hence pins have not been defined, with or without the patch. So I still see no harm to leave the block of -1's, just in case a new board came before upgrading the firmware.

Another alternative could be to replace the last test with an else, ie : else if(pcbVersion == 3){ // TLE5206 by: else { // TLE5206 by default (still not convinced but it works)

Either way, if I were to push my thoughts further, the runtime should stop and yell something like board unrecognized, or version pins configuration unknown.

As for the unused variables, maybe any operation would do the trick, but at the end, it's just a way to help us build better code so I'm more inclined to simplify the code.

[blurfl]

If the famous else is executed, we know that no board was recognized, hence pins have been defined, with or without the patch.

Oops, I see that now. I plead lack of coffee...

Moving the value assignments up to where the auxN variables are defined does eliminate the need for assignments in a final famous else clause. Doesn't that also make the 'if(pcbVersion == 3){ // TLE5206' test in line 268 of the patch unnecessary? All nine auxN pins will have values regardless of which pcbVersion is detected. What about including lines to set pinMode for aux1, aux2 and aux4 but comment them out, for completeness?

If the famous else is executed, we know that no board was recognized, hence pins have been defined, with or without the patch. ... the runtime should stop and yell something like board unrecognized, or version pins configuration unknown.

That would be a good separate patch, maybe create an alarm so Report.cpp could trigger GC to put up a dialog to alert the user. Something along the lines of ALARM_POSITION_LIMIT_ERROR? Should the test live in getPCBVersion()? FWIW, that's another place that could use some attention...

As for the unused variables, maybe any operation would do the trick, but at the end, it's just a way to help us build better code

👍👍

[blurfl]

That all works as advertised, 👍. Just curious, is the test for (pcbVersion >= 3) still needed in line 268? Is there any harm in handling all board versions the same, now that they all have the full seet of aux pin definitions?

[esspe2]

Correct, I think the same! I'm making the change right now, that will also be cleaner.

[blurfl]

Looks very good 💯 Don't forget to cast your own vote at the bottom of 👍the MaslowCommunityGardenRobot's comment!

[MaslowCommunityGardenRobot]

Time is up and we're ready to merge this pull request. Great work!