Remove compiler warnings from libraries we ship

KurtE

Senior Member+
Normally I always have one verbose output for compilation.

And show all compiler warnings, which for those who do not know about these options you can set in the Arduino preferences dialog.

And I normally try to make sure to remove all compiler warnings... However from time to time a few sneak in...

And right now I am seeing a few showing up in a sketch I am working on and I am in the process or removing them.

Sometimes the warnings are not always obvious, example in ST7735 we have code that compiles two unsigned variables and compares to another unsigned variable, and we have a warning:
Code:
yyy:19: warning: comparison between signed and unsigned integer expressions 
   while ((_dma_cnt_sub_frames_per_frame * _dma_buffer_size) != (_count_pixels)) {
                                                             ^
turns out that both: _dma_cnt_sub_frames_per_frame and _dma_buffer_size are uint16_t
and _count_pixels is uint32_t

Fix in this case is I cast the result from the multiply to uint32_t...


In USBHost_t36 we have some code that looks like:
Code:
    uint8_t tag = *p;
    switch (tag & 0x03) { // Short Item data
      case 0: val = 0;
        p++;
        break;
      case 1: val = p[1];
        p += 2;
        break;
      case 2: val = p[1] | (p[2] << 8);
        p += 3;
        break;
      case 3: val = p[1] | (p[2] << 8) | (p[3] << 16) | (p[4] << 24);
        p += 5;
        break;
    }
    if (p > end) break;

    switch (tag & 0xFC) {
      case 0x84: // Report ID (global)
        break;
      case 0x04: // Usage Page (global)
        [COLOR="#FF0000"]usage_page = va[/COLOR]l;
        break;
And it gives us a warning like: warning: 'val' may be used uninitialized in this function

Which is wrong as val will set in all 4 possible cases of a tag & 3...
I obviously can fix several different ways, like: add default: t one of the items in first switch, could just set val=0 when we define the variable and optionally not set it in case 0...

Again I can and will remove these warnings, but thought I would post this as a reminder to myself (and maybe others) to be on the look out for warning in the source code that we ship with Teensyduino .
 
+1 on this. Maybe there are occasional exceptions, but the general rule should be that code compiles without warnings.
 
Changing the optimization level can make these kinds of warnings pop up and disappear too, resulting in a "what the..." moment. switch() statements that set a variable in all cases followed by code that uses that variable to set another can compile cleanly under Faster but throw up 'may be used uninitialized in this function' warnings when re-compiled under Fast.
 
Almost 3 years ago I wrote a script which compiles all the examples from all libraries and logs which have errors and which have warnings.

https://github.com/PaulStoffregen/Verify_All_Arduino_Libraries

Then Teensy 4.0 happened. With so many libraries having compile errors, it was hard to worry about mere compiler warnings. Now almost a year after release, we're finally (with version 1.53) at a point where most of the libraries are ported. I've been running this script again. In the lead-up to 1.53, we talked of several examples from the display libs with errors (but the libraries themselves were ok). That all came about because the script was catching the errors. I also fixed numerous warnings for 1.53, but admittedly the script still uses only the default "Faster" optimization. As we continue improving, I'll expand it to use the other settings.

Farther in the future, I want to expand from merely checking compiler output to actually running automated regression tests for the major libraries on real hardware. I recently designed a first early attempt at hardware to run such tests.

regression_test.jpg

This board integrates 2 USB hubs with per-port power control. But rather than actually shutting off the power to each Teensy, the each port's power control signal asserts the Program signal on Teensy. The general idea is to turn all the ports off, so all 4 boards are held in reset mode as if you were pressing and holding all 4 buttons. Then a script will release the buttons for the board(s) to participate in a particular test, and load the code into each as it releases each Program signal. Most of the tests will probably involve 2 of the boards running and communicating with each other, and of course sending data to the script about whether the test worked.

I integrated several peripherals onto this board, but so far they're untested (but the hub power control is working, at least with that 1 Teensy LC). Near the bottom is a W5500 ethernet which the lower 3 boards can use, and its output connects to the native ethernet pins on the top Teensy 4.1 board. On the right hand side are 3 audio shields and analog switches to connect 1 of their outputs to all 3 inputs. The DAC and MQS outputs are also routed to analog switches, so any of those can drive all 3 audio shield inputs. The 2nd USB hub is meant for testing USB host, where a USB mux allows it to connect to the host port of either Teensy 3.6 or 4.1, and the Teensy LC & 3.2 have USB muxes which allow them to be taken away from the PC's hub and connected to the host port hub. The I2C buses control the audio shields, and also can connect to a FRAM chip (which is one of the I2C parts absolutely requiring repeated start) and also to a BNO055 motion sensor (which is among the most problematic of I2C chips). There's also RS485 and CAN bus chips and other connections between the boards for testing hardware serial with RTS/CTS flow control, and a couple SD cards on the SPI buses.

Eventually I want to create dozens, perhaps hundreds of scripts which each perform some sort of library functional test. Every script will start by turning all 4 boards "off" using the USB hub power control. Then it will turn on specific boards and load each with test code. Several projects already exist to do testing with a single board and parsing its serial output, so we'll probably build on top of that, but using 2 or 3 boards for each test so we can actually do things like have a Teensy 3 synthesize audio waveforms and send them to a Teensy 4 which runs a FFT or other analysis. We might also do things like USB audio and have the test PC capture & analyze data.

Of course this is all in the distant future. But as we get the compiler warnings tamed, I am planning for that future where we have automated regression testing running on real hardware. Maybe someday we'll even get custom GitHub "actions" set up with a private server which runs all pull requests through this sort of extensive automated testing on real hardware.
 
Another +1! I too subscribe to the "all warnings are errors" philosophy.

@silverlock: My opinion of the spurious compiler warnings is that they are also errors, but possibly in the compiler. :) Either way they should be fixed, possibly by documented workarounds in the code.

@PaulStoffregen: This is great work. Do you plan on doing something less random and more aggressive for the Teensy-specific libraries and other libraries under your control?
 
Almost 3 years ago I wrote a script which compiles all the examples from all libraries and logs which have errors and which have warnings.

https://github.com/PaulStoffregen/Verify_All_Arduino_Libraries

Then Teensy 4.0 happened. With so many libraries having compile errors, it was hard to worry about mere compiler warnings. Now almost a year after release, we're finally (with version 1.53) at a point where most of the libraries are ported. I've been running this script again. In the lead-up to 1.53, we talked of several examples from the display libs with errors (but the libraries themselves were ok). That all came about because the script was catching the errors. I also fixed numerous warnings for 1.53, but admittedly the script still uses only the default "Faster" optimization. As we continue improving, I'll expand it to use the other settings.

Farther in the future, I want to expand from merely checking compiler output to actually running automated regression tests for the major libraries on real hardware. I recently designed a first early attempt at hardware to run such tests.

View attachment 21042

This board integrates 2 USB hubs with per-port power control. But rather than actually shutting off the power to each Teensy, the each port's power control signal asserts the Program signal on Teensy. The general idea is to turn all the ports off, so all 4 boards are held in reset mode as if you were pressing and holding all 4 buttons. Then a script will release the buttons for the board(s) to participate in a particular test, and load the code into each as it releases each Program signal. Most of the tests will probably involve 2 of the boards running and communicating with each other, and of course sending data to the script about whether the test worked.

I integrated several peripherals onto this board, but so far they're untested (but the hub power control is working, at least with that 1 Teensy LC). Near the bottom is a W5500 ethernet which the lower 3 boards can use, and its output connects to the native ethernet pins on the top Teensy 4.1 board. On the right hand side are 3 audio shields and analog switches to connect 1 of their outputs to all 3 inputs. The DAC and MQS outputs are also routed to analog switches, so any of those can drive all 3 audio shield inputs. The 2nd USB hub is meant for testing USB host, where a USB mux allows it to connect to the host port of either Teensy 3.6 or 4.1, and the Teensy LC & 3.2 have USB muxes which allow them to be taken away from the PC's hub and connected to the host port hub. The I2C buses control the audio shields, and also can connect to a FRAM chip (which is one of the I2C parts absolutely requiring repeated start) and also to a BNO055 motion sensor (which is among the most problematic of I2C chips). There's also RS485 and CAN bus chips and other connections between the boards for testing hardware serial with RTS/CTS flow control, and a couple SD cards on the SPI buses.

Eventually I want to create dozens, perhaps hundreds of scripts which each perform some sort of library functional test. Every script will start by turning all 4 boards "off" using the USB hub power control. Then it will turn on specific boards and load each with test code. Several projects already exist to do testing with a single board and parsing its serial output, so we'll probably build on top of that, but using 2 or 3 boards for each test so we can actually do things like have a Teensy 3 synthesize audio waveforms and send them to a Teensy 4 which runs a FFT or other analysis. We might also do things like USB audio and have the test PC capture & analyze data.

Of course this is all in the distant future. But as we get the compiler warnings tamed, I am planning for that future where we have automated regression testing running on real hardware. Maybe someday we'll even get custom GitHub "actions" set up with a private server which runs all pull requests through this sort of extensive automated testing on real hardware.

That's really cool to see! For work, we've been adding automated testing to all of our code using the GitLab CI pipeline and Google Test. Works great for code that can run on either Teensy or Linux, but when you need hardware in the loop it gets more fun. I've been using Raspberry Pi 4's as a "server" to the Teensy, so the Raspberry Pi cross-compiles the Teensy code, flashes the Teensy and then cycles power by cycling it's USB port power (essential for getting any attached sensors in a clean state). I have a simple Remote Procedure Call library setup where the "server" commands a test by sending the "client" Teensy the test number to perform. The Teensy executes the test with the hardware and reports back whether it passed or not. It's pretty crude and I'm wanting to develop a better RPC library so we can get tighter integration between GoogleTest running on the server and the hardware test results on the client Teensy.

GitLab's CI interface is really nice though! And I like that we can interface our own servers with it. We restrict our main branch to protect against pushing directly to it and force all code to go through a merge request with a branch, the merge requests require a passed CI pipeline prior to approval.
 
There is this Twitter person :: twitter.com/PaulStoffregen/

Posts things like this on some random interval ... This was there a week ago ...
Paul Stoffregen @PaulStoffregen Jul 12

Do any public microcontroller-based projects use CI to actually run automated tests on the real hardware?
I see @GitHub Actions supports self-hosted runners, but they're not recommended for public repositories. Wondering if anyone's really tried this yet?
 
Yesterday I was playing around some with some of the Adafruit VL6180x sensors, and could not help my self from making changes ;)

So yesterday I did two Pull Requests to the Adafruit library, which have been merged. The first allowed me to set the I2C Address like they do in the VL5... library and the 2nd was to allow me to startup a range operation on all of the sensors and then wait for them to complete... But not important here.

But what is interesting here, is that they have some Automated scripts that are run on your Pull Request. It took me maybe 3 or 4 attempts to make the CLANG and Doxygen parts happy enough to pass. Things like it detected that one of the names in the comments for a function did not match the name of a parameter. Or things like comments in this section were too long, or there were lines that had trailing blanks... I believe it also does probably compile the code as well.

So it might be interesting to see what all their scripts do and if it makes sense for PJRC.

Now back to maybe making a few more PRs to the library.
 
More on the above.

The Adafruit check in stuff uses clang-format...

As @Laydada mentioned in PR:
yeah we recommend installing clang - its pretty easy to install on any platform and comes with clang-format - we use the default setup for clang-format so no special init files needed :)
...
(also clang-format -i will make the changes for you so you dont have to fix them yourself)

So out of curiosity I tried running this on one of my own libraries (ili9341_t3n) on both the header file and main .cpp file.

And it made lots of style changes. Personally not sure it is better or not. But maybe good idea especially if later.

A lot of the changes included:
a) Removing trailing blanks from lines. I used to always have an editor option that did this on saves, but "Got in trouble" wih some projects as it made lots of changes to their files...

b) It feels like it is still back in the teletype days and does not allow comments to go out past column 80... It will wrap those line into multiple lines.

c) Formatting of things like #defines and additional blanks after the define name, to maybe align values, example it changed:
Code:
#define ILI9341_BLACK       0x0000      /*   0,   0,   0 */
#define ILI9341_NAVY        0x000F      /*   0,   0, 128 */
#define ILI9341_DARKGREEN   0x03E0      /*   0, 128,   0 */
#define ILI9341_DARKCYAN    0x03EF      /*   0, 128, 128 */
#define ILI9341_MAROON      0x7800      /* 128,   0,   0 */

to:
Code:
#define ILI9341_BLACK 0x0000       /*   0,   0,   0 */
#define ILI9341_NAVY 0x000F        /*   0,   0, 128 */
#define ILI9341_DARKGREEN 0x03E0   /*   0, 128,   0 */
#define ILI9341_DARKCYAN 0x03EF    /*   0, 128, 128 */
#define ILI9341_MAROON 0x7800      /* 128,   0,   0 */

d) things like how much to indent for lines in structures and classes...

Again what I am wondering is should we strive to have this level of uniformity on all of the Teensyduino libraries?

I know it is sort of a PIA... Also not sure I agree with some of the arbitrariness of the coding styles. But when in Rome...

Thoughts?

P.S - I will probably go ahead and check in the changes for ILI9341_t3n.
 
when i get this error:
"warning: 'val' may be used uninitialized in this function"

i formed a habit of setting the variable to 0 when creating it
int val = 0;

the compiler just doesn't like:
int val;

anyways, I always set it now as a habit :)
 
Again what I am wondering is should we strive to have this level of uniformity on all of the Teensyduino libraries?

My general feeling about whitespace (expect Python) is usually to leave well enough alone, especially if the code has already been widely shared.

Personally I try to keep everything within a 100 char wide window when tabs are formatted as 8 spaces. I try to trim off trailing white space before committing new code to github. But if some trailing spaces exist elsewhere in the file, I generally leave them alone so any commits that aren't new code only affect the lines actually changed in a meaningful way.

I do cringe a bit when someone send a pull request which changes just a few lines, but edits whitespace on many more which aren't related to the change, especially in other files. Sometime I merge those, other times I request a redo or I manually commit just the real changes.
 
Back
Top