Skip to content

Fix warnings and remove -Wno-expansion-to-defined #556

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
matthijskooijman opened this issue Aug 27, 2020 · 0 comments · Fixed by #557
Closed

Fix warnings and remove -Wno-expansion-to-defined #556

matthijskooijman opened this issue Aug 27, 2020 · 0 comments · Fixed by #557

Comments

@matthijskooijman
Copy link
Collaborator

In commit 8575a52, the -Wno-expansion-to-defined was introduced, apparently to prevent a ton of warnings generated by the Atmel CMSIS header files (initially reported here: #290 (comment)):

 #if SAM4_SERIES
 ^~~~~~~~~~~~~~
/home/matthijs/.arduino15/packages/arduino/tools/CMSIS-Atmel/1.2.0/CMSIS/Device/ATMEL/sam.h:563:1: warning: this use of "defined" may not be portable [-Wexpansion-to-defined]

I just tested without this flag (SAMD core 1.8.8, with CMSIS 1.2.0), and it seems the warnings are still there.

Since this warning does point to actual non-portable code, it would be nicer if it was not suppressed, but fixed in the CMSIS instead.

The actual problem seems to be that they use a macro that contains defined(), e.g. something like #define BAR defined(FOO) and then check that later (#if BAR). In the Atmel CMSIS code, this is a bit more complex, because they use a part_is_defined intermediate macro: https://github.com/arduino/ArduinoModule-CMSIS-Atmel/blob/46ab1021146152a64caf1ddbb837d8181b8faa35/CMSIS-Atmel/CMSIS/Device/ATMEL/sam.h#L32

E.g.:

#define part_is_defined(part) (defined(__ ## part ## __))
#define SAMG55J1 ( \
    part_is_defined( SAMG55J19 ) )

I think the portable way to write this, is to write:

#if defined(SAMG55J19)
#define SAMG55J1 1
#endif

This might also need an else to define it as 0, but that's not needed if the macro is only used from the preprocessor I think. There's 150 instances of this macro, but all in the same file, so might be fixable automatically.

I also looked to see if newer versions of the Atmel CMSIS library still have this, but couldn't quite find this particular library with this layout. I looked inside the "ASF standalone framework 3.48", but that seemingly contains the same header files and components, but there is no "sam.h" that figures out what header file to include, only e.g. "samd21.h" that figures out which of the SAMD21 headers to include. To use those, I think we should let our own headers decide which family header to include exactly. I also looked at the Microchips packs, which is pretty much the same thing, but downloadable per family.

So if we'd upgrade this CMSIS stuff, I guess we could just pick out the families we need and include the right one directly.

Thinking on this, we could even do that now already and bypass the problematic sam.h header entirely. Looking at boards.txt, all supported boards are SAMD21 anyway, so the below diff actually fixes this warning right now (while I was there, I also replaced the "" with <> in the includes, which is not actualy needed for this fix, but is a bit more correct):

diff --git a/cores/arduino/Arduino.h b/cores/arduino/Arduino.h
index c374c96..a40a5e4 100644
--- a/cores/arduino/Arduino.h
+++ b/cores/arduino/Arduino.h
@@ -45,7 +45,7 @@ extern "C"{
 #endif // __cplusplus
 
 // Include Atmel headers
-#include "sam.h"
+#include <samd.h>
 
 #include "wiring_constants.h"
 
diff --git a/cores/arduino/SERCOM.h b/cores/arduino/SERCOM.h
index 6f855af..cbc3a05 100644
--- a/cores/arduino/SERCOM.h
+++ b/cores/arduino/SERCOM.h
@@ -19,7 +19,7 @@
 #ifndef _SERCOM_CLASS_
 #define _SERCOM_CLASS_
 
-#include "sam.h"
+#include <samd.h>
 
 #define SERCOM_FREQ_REF      48000000
 #define SERCOM_NVIC_PRIORITY ((1<<__NVIC_PRIO_BITS) - 1)
diff --git a/cores/arduino/WVariant.h b/cores/arduino/WVariant.h
index bbe2e0c..d44a0e2 100644
--- a/cores/arduino/WVariant.h
+++ b/cores/arduino/WVariant.h
@@ -19,7 +19,7 @@
 #pragma once
 
 #include <stdint.h>
-#include "sam.h"
+#include <samd.h>
 
 #ifdef __cplusplus
 extern "C" {
diff --git a/cores/arduino/cortex_handlers.c b/cores/arduino/cortex_handlers.c
index a910d08..e125b6e 100644
--- a/cores/arduino/cortex_handlers.c
+++ b/cores/arduino/cortex_handlers.c
@@ -16,7 +16,7 @@
   Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
 */
 
-#include <sam.h>
+#include <samd.h>
 #include <variant.h>
 #include <stdio.h>
 
diff --git a/cores/arduino/startup.c b/cores/arduino/startup.c
index d66bfa8..3f990ab 100644
--- a/cores/arduino/startup.c
+++ b/cores/arduino/startup.c
@@ -16,7 +16,7 @@
   Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
 */
 
-#include "sam.h"
+#include <samd.h>
 #include "variant.h"
 
 #include <stdio.h>
diff --git a/cores/arduino/wiring_private.h b/cores/arduino/wiring_private.h
index ce64e2d..1393237 100644
--- a/cores/arduino/wiring_private.h
+++ b/cores/arduino/wiring_private.h
@@ -27,7 +27,7 @@ extern "C" {
 #endif
 
 // Includes Atmel CMSIS
-#include "sam.h"
+#include <samd.h>
 
 #include "wiring_constants.h"
 
diff --git a/platform.txt b/platform.txt
index 0a860df..7e6f881 100644
--- a/platform.txt
+++ b/platform.txt
@@ -28,8 +28,8 @@ version=1.8.5
 compiler.warning_flags=-w
 compiler.warning_flags.none=-w
 compiler.warning_flags.default=
-compiler.warning_flags.more=-Wall -Wno-expansion-to-defined
-compiler.warning_flags.all=-Wall -Wextra -Wno-expansion-to-defined
+compiler.warning_flags.more=-Wall
+compiler.warning_flags.all=-Wall -Wextra
 
 # EXPERIMENTAL feature: optimization flags
 #  - this is alpha and may be subject to change without notice
matthijskooijman added a commit to matthijskooijman/ArduinoCore-samd that referenced this issue Aug 27, 2020
The sam.h file uses some non-portable macros that raise a warning in
newer gcc version. This warning was supressed in commit 8575a52 (Add
-Wno-expansion-to-defined compile warning flag), but this is not ideal.

However, since the only thing sam.h does is figure out what CPU is
selected and include the right family header, and we always use SAMD21
CPUs, the only thing sam.h does is include samd.h.

So we can easily bypass then and include samd.h directly.

This fixes the first part of arduino#556.
matthijskooijman added a commit to matthijskooijman/ArduinoCore-samd that referenced this issue Aug 27, 2020
Now that we no longer include sam.h, this warning is no longer triggered
in normal builds, so there is no longer a need to supress it.

This fixes arduino#556.
matthijskooijman added a commit to matthijskooijman/ArduinoCore-samd that referenced this issue Nov 26, 2020
The sam.h file uses some non-portable macros that raise a warning in
newer gcc version. This warning was supressed in commit 8575a52 (Add
-Wno-expansion-to-defined compile warning flag), but this is not ideal.

However, since the only thing sam.h does is figure out what CPU is
selected and include the right family header, and we always use SAMD21
CPUs, the only thing sam.h does is include samd.h.

So we can easily bypass then and include samd.h directly.

This fixes the first part of arduino#556.
matthijskooijman added a commit to matthijskooijman/ArduinoCore-samd that referenced this issue Nov 26, 2020
Now that we no longer include sam.h, this warning is no longer triggered
in normal builds, so there is no longer a need to supress it.

This fixes arduino#556.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant