Skip to content
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.

Sign up for GitHub

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

wrong assumption, that pin > PINS_COUNT cannot yield valid pins for interrupts #9

Closed
wants to merge 2 commits into from
Closed

wrong assumption, that pin > PINS_COUNT cannot yield valid pins for interrupts #9

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad53f65
fix wrong assumption
polygamma Oct 12, 2018
d39fa7e
this commit fixes broken serial connections after sleeping
polygamma Oct 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions src/samd/ArduinoLowPower.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ void ArduinoLowPowerClass::idle(uint32_t millis) {
void ArduinoLowPowerClass::sleep() {
bool restoreUSBDevice = false;
if (SERIAL_PORT_USBVIRTUAL) {
SERIAL_PORT_USBVIRTUAL.flush();
USBDevice.standby();
} else {
USBDevice.detach();
Expand All @@ -29,6 +30,9 @@ void ArduinoLowPowerClass::sleep() {
if (restoreUSBDevice) {
USBDevice.attach();
}
if (SERIAL_PORT_USBVIRTUAL) {
SERIAL_PORT_USBVIRTUAL.clear();
}
}

void ArduinoLowPowerClass::sleep(uint32_t millis) {
Expand Down Expand Up @@ -57,7 +61,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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Author

@polygamma polygamma Oct 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: https://github.com/polygamma/ArduinoCore-samd/commit/1acb64aecfacb1012aed0d6f5df65526d2e5e37d


if (in == NOT_AN_INTERRUPT || in == EXTERNAL_INT_NMI) {
// check for external wakeup sources
// RTC library should call this API to enable the alarm subsystem
switch (pin) {
Expand All @@ -69,10 +75,6 @@ void ArduinoLowPowerClass::attachInterruptWakeup(uint32_t pin, voidFuncPtr callb
return;
}

EExt_Interrupts in = g_APinDescription[pin].ulExtInt;
if (in == NOT_AN_INTERRUPT || in == EXTERNAL_INT_NMI)
return;

//pinMode(pin, INPUT_PULLUP);
attachInterrupt(pin, callback, mode);

Expand Down