MaslowCNC / Firmware

Maslow Firmware
GNU General Public License v3.0
263 stars 134 forks source link

gcc warnings Part II #413

Closed aprospero closed 6 years ago

aprospero commented 6 years ago

Hmmm, maybe I was a bit hasty immediately deleting the last pull requests branch. I had another thought about your proposal to include the warnings into the Travis build. You're right, it would be a good move. So I had a look at it. Travis just starts a platformIO build and I haven't been in touch with that yet. But on a second glance the next step seems obvious.

--- platformio.ini 2018-03-22 01:35:44.031037400 +0100 +++ platformio.ini 2018-03-22 01:35:44.031037500 +0100 @@ -22,7 +22,7 @@ framework = arduino extra_scripts = platformio/simavr_env.py build_unflags = -Os -build_flags = -g -O0 -DSIMAVR -DFAKE_SERVO +build_flags = -Wall -g -O0 -DSIMAVR -DFAKE_SERVO monitor_port = /tmp/simavr-uart0

I think it is important to deny Travis reporting success if warnings occur during build and I don't know how or if I even manually have to get Travis to report popped up warnings. With a bit of luck this will work from scratch.

BarbourSmith commented 6 years ago

Great! I'll merge this and let's find out 😀

aprospero commented 6 years ago

Hehe, learning by failure is the most effective strategy. :)

And it fails... - a bit - sort of...

I do not claim to completely understand the reasons behind disabling compiler optimizations in platformio.ini ('build_unflags = -Os' and 'build_flags = ... -O0 ...'). It is generally not recommended to do this on Arduino but it may be useful when it comes to debugging on a simulation target. However, it (intentionally) generates a lot of compiler warnings in the target library layer - not in maslow code. That's exactly the opposite of what we wanted to achieve.

I'm not sure if this pull request is still worth a merge. With the warnings in code we haven't our hands on the original incentive breaks into pieces.

Nevertheless, when I was tinkering with Atom/PlatformIO another unused Variable popped up in conditionally compiled code. I fixed it in another commit. It's up to you to decide if this is still worth the merge. Please let me know if you want to revert this pull request and/or if you want to merge the follow-up commit. I'll open up another pull request then.

MaslowCommunityGardenRobot commented 6 years ago

Is there another way to learn? 😀

Let's fix the other unused variable for sure, no reason to leave that in there.

As far as this PR I vote that we revert it if we're not going to use the changes

Edit: Darn it...this is Bar...I'm just logged in as a different account 😬