wrong assumption, that pin > PINS_COUNT cannot yield valid pins for interrupts
#9
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pin > PINS_COUNT may yield valid pins for interrupts
facchinm
reviewed
Oct 15, 2018
| @@ -57,7 +57,9 @@ void ArduinoLowPowerClass::setAlarmIn(uint32_t millis) { | |||
|
|
|||
| void ArduinoLowPowerClass::attachInterruptWakeup(uint32_t pin, voidFuncPtr callback, uint32_t mode) { | |||
|
|
|||
| if (pin > PINS_COUNT) { | |||
| EExt_Interrupts in = g_APinDescription[pin].ulExtInt; | |||
There was a problem hiding this comment.
While better than the PINS_COUNT approach, you could easily read out of buffer using this code (RTC, for example, is hardcoded as 0xFF here.
I'd rather go for something like this, which is already included in beta branch, or a compile time calculation like this
From 5748e473f61d828a518164ad82660cb13eb7b150 Mon Sep 17 00:00:00 2001
From: Martino Facchin <[email protected]>
Date: Wed, 18 Apr 2018 10:26:54 +0200
Subject: [PATCH] TEST: automatically define PINS_COUNT from g_APinDescription
size
---
variants/mkrwifi1010/variant.cpp | 6 ++++++
variants/mkrwifi1010/variant.h | 12 +++++++++++-
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/variants/mkrwifi1010/variant.cpp b/variants/mkrwifi1010/variant.cpp
index cf155c8..db1dfdd 100644
--- a/variants/mkrwifi1010/variant.cpp
+++ b/variants/mkrwifi1010/variant.cpp
@@ -167,6 +167,12 @@ const PinDescription g_APinDescription[] = {
const void* g_apTCInstances[TCC_INST_NUM + TC_INST_NUM]={ TCC0, TCC1, TCC2, TC3, TC4, TC5 };
+extern "C" {
+ unsigned int PINCOUNT_fn() {
+ return (sizeof(g_APinDescription) / sizeof(g_APinDescription[0]));
+ }
+}
+
// Multi-serial objects instantiation
SERCOM sercom0(SERCOM0);
SERCOM sercom1(SERCOM1);
diff --git a/variants/mkrwifi1010/variant.h b/variants/mkrwifi1010/variant.h
index ed4aac4..3cfa286 100644
--- a/variants/mkrwifi1010/variant.h
+++ b/variants/mkrwifi1010/variant.h
@@ -22,6 +22,7 @@
#define ARDUINO_SAMD_VARIANT_COMPLIANCE 10610
#include <WVariant.h>
+#include "stdlib.h"
// General definitions
// -------------------
@@ -36,7 +37,7 @@
// ----
// Number of pins defined in PinDescription array
-#define PINS_COUNT (26u)
+#define PINS_COUNT (PINCOUNT_fn())
#define NUM_DIGITAL_PINS (15u)
#define NUM_ANALOG_INPUTS (7u)
#define NUM_ANALOG_OUTPUTS (1u)
@@ -188,6 +189,15 @@ extern Uart Serial2;
#endif // __cplusplus
+#ifdef __cplusplus
+extern "C" {
+#endif
+unsigned int PINCOUNT_fn();
+#ifdef __cplusplus
+}
+#endif
+
+
// These serial port names are intended to allow libraries and architecture-neutral
// sketches to automatically default to the correct port name for a particular type
// of use. For example, a GPS module would normally connect to SERIAL_PORT_HARDWARE_OPEN,There was a problem hiding this comment.
You are right, I haven't thought this through completely. Going to revert that commit, and will be putting your suggested solution into every variant.cpp and variant.h instead.
There was a problem hiding this comment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
e.g. the
Arduino MKRFOX1200has theSIGFOX_EVENT_PINon 33, whereasPINS_COUNTyields 26, which leads to the SigFox library being unable to register interrupts for wakeups based on those events.Compare with: https://github.com/arduino-libraries/SigFox/blob/master/src/SigFox.cpp#L170
This pull request changes the logic to detect invalid pins slightly, so that using the
SIGFOX_EVENT_PINfor interrupts is now possible.Edit: I have appended something else. This commit together with this also fixes this, the problems with Serial not working after sleeping.