Closed ObviousInRetrospect closed 1 year ago
ObviousInRetrospect
commented
2 years ago Yeah I meant is it in a state where you want to attach it here as a zip to point people at (and to let @SpenceKonde merge)?
I'm 95% convinced this was the remaining problem I was having with pwr_down and you have now fixed the library to work flawlessly in all sleep modes (and overnight testing will cover the remaining 5%). Thanks again!
MX682X
commented
2 years ago src.zip I guess this is the release candidate then. It's certainly more reliable then before anyway. Got slightly bigger though, hopefully we can offset that by reducing the Stream RAM needs.
On a side note, has anyone seen any library use Prints write error functions? because that's another two RAM bytes....
int write_error;
protected:
void setWriteError(int err = 1) { write_error = err; }
public:
Print() : write_error(0) {}
int getWriteError() { return write_error; }
void clearWriteError() { setWriteError(0); }with 1.2m pass and no errors this is looking fixed
CRC valid
fail: 0 pass: 1204684
SpenceKonde
commented
2 years ago This is excellent work.
google arduino setWriteError and tell google that you really meant that not with spaces between the words in the name of the function. You will see that it is is widely used and almost never passed a value.
I will contact my craftsmen at once to have them produce an extra mvp award from the finest materials, as we clearly have two individuals to deliver them to. "Cat, you hear that? We need a second trophy made for this month. Don't give me that about lead time. I see you snoozing on the job all day and I see how fast you shed, if you put that energy towards doing your job here, you would have no trouble meeting this deadline... and if you expect to get cat food next month, I will see two awards ready for the awards ceremony on the august 30! Do I make myself clear?"
MX682X
commented
2 years ago @ObviousInRetrospect after I managed to figure out how to optimize the slave interrupt routine (by convincing the compiler to use the Y-Register and ldd/std instead of sts/lds), I was able to change how slaveTransactionOpen() works internally without bloating the code size. I'd apreciate it if you could run a test without the cli/sei guards.
this version uses a variable to remember when an address match interrupt occurs, so slaveTrasactionOpen won't return 0 between address and stop interrupt and thus should not allow the system to sleep.
src.zip
ObviousInRetrospect
commented
2 years ago Happy to run the test (and will start it shortly) although I still think there is a highly improbable highly infrequent race condition without the cli/sei guard.
The address interrupt can still fire between or during the return of slaveTransactionOpen() and the sleep instruction.
Unless I am missing something the cli/sei is correct and excludes a race condition that will cause extremely infrequent lock ups. I still am not sure I understand why the failure that happens is as bad as it is (slave fails until reset) in that I have a PIT interrupt causing non twi wake frequently - I would kind of expect the transaction the slave fell asleep on to fail but the transaction open to return true and prevent sleep. I think there might still be a missing recovery transition.
MX682X
commented
2 years ago You are right. But I think reducing the time it might happen is desirable anyway.
I've brainstormed a bit. What if the TWI ISR will set the sleep mode to IDLE at Address IRQ and restore it at Stop IRQ? This might even allow people to put the AVR into IDLE sleep between data IRQs and thus further minimizing power consumption, instead of letting the CPU do nothing...
ObviousInRetrospect
commented
2 years ago The savings from sleep mode idle are very minor, I wouldn't jump through hoops to sit in idle while a slave transaction is open vs busy-wait.
Or are you suggesting it not for the purpose of allowing idle sleep for that limited time but instead for the purpose of making it safe to sleep regardless of TWI state and eliminating the need to call slaveTransactionOpen() at all?
The second thing, if it works, seems like a win.
MX682X
commented
2 years ago For making it save. the idle power saving is just a plus
ObviousInRetrospect
commented
2 years ago it might actually make sleep safe without the check - I've tested without the call to slaveTransactionOpen in idle and it seemed to work fine
MX682X
commented
2 years ago src.zip alright, release candidate finished and tested on a 1614. Wire library now guards the SLEEP itself. It buffers the SMODE, but keeps the SEN unchanged. This allows the chip to sleep in IDLE while it waits for the next data interrupt, if sleep was enabled beforehand by user. Here is a nice screenshot of my Oscilloscope, displaying a voltage drop across a 100 Ohm resistor. Would appreciate if you, @ObviousInRetrospect , could also run some tests. no need for Wire.slaveTransactionOpen() aswell.
Code on 1614:
#include <avr/sleep.h>
#include "Wire.h"
#include <CRC32.h>
uint8_t inBuffer[32] = {0};
uint8_t received = 0;
void setup() {
Wire.onReceive(srmw);
Wire.onRequest(swmr);
Wire.begin(32);
pinMode(PIN_PA1, OUTPUT);
digitalWriteFast(PIN_PA1, HIGH);
pinMode(PIN_PA2, INPUT_PULLUP);
attachInterrupt(digitalPinToInterrupt(PIN_PA2), swreset, LOW);
pinMode(PIN_PA3, INPUT_PULLUP);
set_sleep_mode(SLEEP_MODE_PWR_DOWN);
sleep_enable();
}
void swreset(void) {
CCP = CCP_IOREG_gc;
RSTCTRL.SWRR = 0x01;
}
void loop() {
digitalWriteFast(PIN_PA1, LOW);
sleep_cpu();
digitalWriteFast(PIN_PA1, HIGH);
}
void srmw(int16_t num) {
Wire.readBytes(inBuffer, num);
received = 1;
}
void swmr(void) {
if (received == 0) {
for (uint8_t i = 0; i < 28; i++) {
inBuffer[i] = 32 - i;
}
}
received = 0;
uint32_t crc = CRC32::calculate(inBuffer, 28);
uint32_t* crcPos = (uint32_t*)&inBuffer[28];
(*crcPos) = crc;
Wire.write(inBuffer, 32);
}
ObviousInRetrospect
commented
2 years ago @MX682X I typically run the slave test off a DX and the master off a tiny. Unless I hear otherwise that is 1.4.10 and 2.6.1. Can probably reverse that if you wanted.
MX682X
commented
2 years ago the version doesn|t matter. just replace the Wire files with the ones I provided
ObviousInRetrospect
commented
2 years ago Release candidate has some regressions. Mostly warnings but I still use a chip that doesn't have a VPORTB.
Poking around to see how I fixed that last time. (got side tracked by an IDE 2.0 bug)
/Users/user/Library/Arduino15/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi_pins.c: In function 'TWI1_usePullups':
/Users/user/Library/Arduino15/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi_pins.c:512:11: warning: unused variable 'portmux' [-Wunused-variable]
uint8_t portmux = PORTMUX.TWIROUTEA & PORTMUX_TWI1_gm;
^~~~~~~
/Users/user/Library/Arduino15/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi_pins.c: In function 'TWI1_checkPinLevel':
/Users/user/Library/Arduino15/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi.c: In function 'TWI_MasterWrite':
/Users/user/Library/Arduino15/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi_pins.c:552:16: error: 'VPORTB' undeclared (first use in this function); did you mean 'VPORTA'?
vport = &VPORTB;
^~~~~~
VPORTA
/Users/user/Library/Arduino15/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi_pins.c:552:16: note: each undeclared identifier is reported only once for each function it appears in
/Users/user/Library/Arduino15/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi.c:320:12: warning: variable 'timeout' set but not used [-Wunused-but-set-variable]
uint16_t timeout = 0;
^~~~~~~
/Users/user/Library/Arduino15/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi_pins.c:547:13: warning: unused variable 'portmux' [-Wunused-variable]
uint8_t portmux = PORTMUX.TWIROUTEA & PORTMUX_TWI1_gm;
^~~~~~~
/Users/user/Library/Arduino15/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi.c: In function 'TWI_MasterRead':
/Users/user/Library/Arduino15/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi.c:420:14: warning: variable 'timeout' set but not used [-Wunused-but-set-variable]
uint16_t timeout = 0;
^~~~~~~
exit status 1
Error compiling for board AVR DB-series (no bootloader).here is the offending code
#if (defined (VPORTB) && defined (VPORTF))
((portmux == PORTMUX_TWI1_ALT2_gc) ? (vport = &VPORTB) : (vport = &VPORTF));
#elif defined (VPORTA)
vport = &VPORTB;
#elif defined (VPORTC)
vport = &VPORTF;
#elselooking at the portmux to see if I can figure out what it should be.
ObviousInRetrospect
commented
2 years ago got it. never uses VPORTA. changing
#elif defined (VPORTA)to
#elif defined (VPORTB)Not sure why you are guarding VPORTF with defined VPORTC though. All DX have VPORTC afaik.
MX682X
commented
2 years ago Copy paste errors... the bane of many programmers. Also I noticed, after writing the message that timeout was disabled, that's where the unused variables come from
ObviousInRetrospect
commented
2 years ago Here is the full patch I applied to the RC:
--- /tmp/twi_pins.c 2022-09-17 08:07:57.000000000 -0700
+++ /Users/user/Library/Arduino15/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi_pins.c 2022-09-17 08:11:28.000000000 -0700
@@ -509,7 +509,9 @@
void TWI1_usePullups() {
- uint8_t portmux = PORTMUX.TWIROUTEA & PORTMUX_TWI1_gm;
+ #if ( (defined (PORTB) && defined (PORTF)) || defined(TWI_DUALCTRL) )
+ uint8_t portmux = PORTMUX.TWIROUTEA & PORTMUX_TWI1_gm;
+ #endif
PORT_t *port;
#if (defined (PORTB) && defined (PORTF))
((portmux == PORTMUX_TWI1_ALT2_gc) ? (port = &PORTB) : (port = &PORTF));
@@ -544,11 +546,13 @@
//Check if TWI1 Master pins have a HIGH level: Bit0 = SDA, Bit 1 = SCL
uint8_t TWI1_checkPinLevel(void) {
#if defined(DXCORE) /* Dx-series */
- uint8_t portmux = PORTMUX.TWIROUTEA & PORTMUX_TWI1_gm;
+ #if ((defined (VPORTB) && defined (VPORTF)) || defined(__AVR_DD__))
+ uint8_t portmux = PORTMUX.TWIROUTEA & PORTMUX_TWI1_gm;
+ #endif
VPORT_t *vport;
#if (defined (VPORTB) && defined (VPORTF))
((portmux == PORTMUX_TWI1_ALT2_gc) ? (vport = &VPORTB) : (vport = &VPORTF));
- #elif defined (VPORTA)
+ #elif defined (VPORTB)
vport = &VPORTB;
#elif defined (VPORTC)
vport = &VPORTF;and
--- /tmp/twi.c 2022-09-17 08:13:45.000000000 -0700
+++ /Users/user/Library/Arduino15/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi.c 2022-09-17 08:20:04.000000000 -0700
@@ -317,7 +317,10 @@
uint8_t currentSM;
uint8_t currentStatus;
uint8_t dataWritten = 0;
- uint16_t timeout = 0;
+ #if defined(TWI_TIMEOUT_ENABLE)
+ uint16_t timeout = 0;
+ #endif
+
if (((module->MSTATUS & TWI_BUSSTATE_gm) == TWI_BUSSTATE_UNKNOWN_gc) || // If the bus was not initialized
@@ -350,7 +353,9 @@
if (currentSM == TWI_BUSSTATE_IDLE_gc) { // Bus has not sent START yet and is not BUSY
module->MADDR = ADD_WRITE_BIT(_data->_clientAddress);
- timeout = 0;
+ #if defined(TWI_TIMEOUT_ENABLE)
+ timeout = 0;
+ #endif
} else if (currentSM == TWI_BUSSTATE_OWNER_gc) { // Address was sent, host is owner
if (currentStatus & TWI_WIF_bm) { // data sent
if (currentStatus & TWI_RXACK_bm) { // AND the RXACK bit is set, last byte has failed
@@ -361,7 +366,9 @@
if (dataWritten < (*txHead)) { // check if there is data to be written
module->MDATA = txBuffer[dataWritten]; // Writing to the register to send data
dataWritten++; // data was Written
- timeout = 0; // reset timeout
+ #if defined(TWI_TIMEOUT_ENABLE)
+ timeout = 0;
+ #endif // reset timeout
} else { // else there is no data to be written
break; // TX finished, leave loop, error is still TWI_NO_ERR
}
@@ -417,7 +424,9 @@
uint8_t currentSM;
uint8_t currentStatus;
uint8_t command = 0;
- uint16_t timeout = 0;
+ #if defined(TWI_TIMEOUT_ENABLE)
+ uint16_t timeout = 0;
+ #endif
module->MADDR = ADD_READ_BIT(_data->_clientAddress); // Send Address with read bit
@@ -457,8 +466,9 @@
if (dataRead < BUFFER_LENGTH) { // Buffer still free
rxBuffer[dataRead] = module->MDATA; // save byte in the Buffer.
dataRead++; // increment read counter
- timeout = 0; // reset timeout
-
+ #if defined(TWI_TIMEOUT_ENABLE)
+ timeout = 0; // reset timeout
+ #endif
if (dataRead < bytesToRead) { // expecting more bytes, so
module->MCTRLB = TWI_MCMD_RECVTRANS_gc; // send an ACK so the Slave so it can send the next byte
} else { DD series has only TWI0, so there is technically no point in doing || defined(__AVR_DD__) in TWI1 check pins
also, this
#elif defined (VPORTC)
vport = &VPORTF;has to be
#elif defined (VPORTF)
vport = &VPORTF;ok, amending that to:
--- /tmp/twi_pins.c 2022-09-17 08:07:57.000000000 -0700
+++ /Users/user/Library/Arduino15/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi_pins.c 2022-09-17 08:33:44.000000000 -0700
@@ -509,7 +509,9 @@
void TWI1_usePullups() {
- uint8_t portmux = PORTMUX.TWIROUTEA & PORTMUX_TWI1_gm;
+ #if ( (defined (PORTB) && defined (PORTF)) || defined(TWI_DUALCTRL) )
+ uint8_t portmux = PORTMUX.TWIROUTEA & PORTMUX_TWI1_gm;
+ #endif
PORT_t *port;
#if (defined (PORTB) && defined (PORTF))
((portmux == PORTMUX_TWI1_ALT2_gc) ? (port = &PORTB) : (port = &PORTF));
@@ -544,24 +546,20 @@
//Check if TWI1 Master pins have a HIGH level: Bit0 = SDA, Bit 1 = SCL
uint8_t TWI1_checkPinLevel(void) {
#if defined(DXCORE) /* Dx-series */
- uint8_t portmux = PORTMUX.TWIROUTEA & PORTMUX_TWI1_gm;
+ #if (defined (VPORTB) && defined (VPORTF))
+ uint8_t portmux = PORTMUX.TWIROUTEA & PORTMUX_TWI1_gm;
+ #endif
VPORT_t *vport;
#if (defined (VPORTB) && defined (VPORTF))
((portmux == PORTMUX_TWI1_ALT2_gc) ? (vport = &VPORTB) : (vport = &VPORTF));
- #elif defined (VPORTA)
+ #elif defined (VPORTB)
vport = &VPORTB;
- #elif defined (VPORTC)
+ #elif defined (VPORTF)
vport = &VPORTF;
#else
#warning "Something went wrong. No TWI1 related Port was defined"
return 0;
#endif
-
- #if defined(__AVR_DD__)
- if (3 == portmux) {
- return ((vport->IN & 0x03);
- } else
- #endif
{
return ((vport->IN & 0x0C) >> 2);
}wasn't thinking about whether the TWI1 code should be on DD there just saw a reference guarded by DD that I matched.
installing mTC (2.6.0) via boards manager seems to have removed my DxCore's python (???)
java.io.IOException: Cannot run program "/Users/user/Library/Arduino15/packages/DxCore/hardware/megaavr/1.4.10/tools/python3/python3": error=2, No such file or directorytotally trying to get this test running but my arduino environment is in an ... interesting state.
MX682X
commented
2 years ago mTC 2.6.0 or 2.6.1? Asking since 2.6.1 was released a couple hours ago
ObviousInRetrospect
commented
2 years ago I installed 2.6.0 via Boards Manager, discovered the azduino5 issue and as a result Spence pulled it. I fixed the azduino5 issue in place. I then brought in head of tree (2.6.1) via manual installation.
2.6.1 still isn't in the json that drives boards manager.
ObviousInRetrospect
commented
2 years ago Ok, brought up a portable running release dxc only. Something is failing pretty horribly but I am not convinced its your new code. Working on switching the master back to mTC 2.5.11 as backing down to the previously working wire library on DxC didn't fix the problem.
ObviousInRetrospect
commented
2 years ago ok got the tests running again.
initial (o(10k) iteration) smoke tests show: new code works properly in SLEEP_MODE_IDLE without a check for slaveTransactionOpen new code works properly in SLEEP_MODE_STANDBY without a check for slaveTransactionOpen new code fails miserably in SLEEP_MODE_PWR_DOWN without a check for slaveTransactionOpen (successes are rare failures are 99% of cases) new code works properly in SLEEP_MODE_PWR_DOWN with a check for slaveTransactionOpen
working on double checking those results, then will track down whether mTC2.6.1 is causing a problem on the master side (no evidence yet).
ObviousInRetrospect
commented
2 years ago incidentally, now getting the following pile-o-warnings:
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi.h:169:8: warning: type 'struct twiDataBools' violates the C++ One Definition Rule [-Wodr]
struct twiDataBools { // using a struct so the compiler can use skip if bit is set/cleared
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi.h:169:8: note: a different type is defined in another translation unit
struct twiDataBools { // using a struct so the compiler can use skip if bit is set/cleared
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi.h:170:27: note: the first difference of corresponding definitions is field '_slaveTrStatus'
uint8_t _slaveTrStatus: 2;
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi.h:170:27: note: a field with different name is defined in another translation unit
uint8_t _slaveTrStatus: 2;
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi.h:179:8: warning: type 'struct twiData' violates the C++ One Definition Rule [-Wodr]
struct twiData {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi.h:178:8: note: a different type is defined in another translation unit
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi.h:181:23: note: the first difference of corresponding definitions is field '_bools'
struct twiDataBools _bools; // the structure to hold the bools for the class
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi.h:180:23: note: a field of same name but different type is defined in another translation unit
TWI_t *_module;
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi.h:169:8: note: type 'struct twiDataBools' itself violates the C++ One Definition Rule
struct twiDataBools { // using a struct so the compiler can use skip if bit is set/cleared
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:62:7: warning: type 'struct TwoWire' violates the C++ One Definition Rule [-Wodr]
class TwoWire: public Stream {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/wire.h:62:7: note: a different type is defined in another translation unit
class TwoWire: public Stream {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:64:13: note: the first difference of corresponding definitions is field 'vars'
twiData vars; // We're using a struct to reduce the amount of parameters that have to be passed.
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/wire.h:64:13: note: a field of same name but different type is defined in another translation unit
twiData vars; // We're using a struct to reduce the amount of parameters that have to be passed.
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi.h:179:8: note: type 'struct twiData' itself violates the C++ One Definition Rule
struct twiData {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/twi.h:178:8: note: the incompatible type is defined here
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:91:20: warning: 'write' violates the C++ One Definition Rule [-Wodr]
virtual size_t write(uint8_t);
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:346:8: note: implicit this pointer type mismatch
size_t TwoWire::write(uint8_t data) {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:62:7: note: type 'struct TwoWire' itself violates the C++ One Definition Rule
class TwoWire: public Stream {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:346:8: note: 'write' was previously declared here
size_t TwoWire::write(uint8_t data) {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:346:8: note: code may be misoptimized unless -fno-strict-aliasing is used
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:99:13: warning: 'getBytesRead' violates the C++ One Definition Rule [-Wodr]
uint8_t getBytesRead(void);
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:593:9: note: implicit this pointer type mismatch
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:62:7: note: type 'struct TwoWire' itself violates the C++ One Definition Rule
class TwoWire: public Stream {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:593:9: note: 'getBytesRead' was previously declared here
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:593:9: note: code may be misoptimized unless -fno-strict-aliasing is used
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:94:17: warning: 'read' violates the C++ One Definition Rule [-Wodr]
virtual int read(void);
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:446:5: note: implicit this pointer type mismatch
int TwoWire::read(void) {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:62:7: note: type 'struct TwoWire' itself violates the C++ One Definition Rule
class TwoWire: public Stream {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:446:5: note: 'read' was previously declared here
int TwoWire::read(void) {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:446:5: note: code may be misoptimized unless -fno-strict-aliasing is used
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:102:13: warning: 'enableDualMode' violates the C++ One Definition Rule [-Wodr]
void enableDualMode(bool fmp_enable); // Moves the Slave to dedicated pins
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:632:6: note: implicit this pointer type mismatch
*@param bool fmp_enable - set true if the TWI module has to expect a high
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:62:7: note: type 'struct TwoWire' itself violates the C++ One Definition Rule
class TwoWire: public Stream {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:632:6: note: 'enableDualMode' was previously declared here
*@param bool fmp_enable - set true if the TWI module has to expect a high
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:632:6: note: code may be misoptimized unless -fno-strict-aliasing is used
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:73:10: warning: 'begin' violates the C++ One Definition Rule [-Wodr]
void begin(); // all attempts to make these look prettier were rejected by astyle, and it's not worth disabling linting over.
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:163:6: note: implicit this pointer type mismatch
void TwoWire::begin(void) {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:62:7: note: type 'struct TwoWire' itself violates the C++ One Definition Rule
class TwoWire: public Stream {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:163:6: note: 'begin' was previously declared here
void TwoWire::begin(void) {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:163:6: note: code may be misoptimized unless -fno-strict-aliasing is used
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:79:10: warning: 'beginTransmission' violates the C++ One Definition Rule [-Wodr]
void beginTransmission(uint8_t address);
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:283:6: note: implicit this pointer type mismatch
void TwoWire::beginTransmission(uint8_t address) {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:62:7: note: type 'struct TwoWire' itself violates the C++ One Definition Rule
class TwoWire: public Stream {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:283:6: note: 'beginTransmission' was previously declared here
void TwoWire::beginTransmission(uint8_t address) {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:283:6: note: code may be misoptimized unless -fno-strict-aliasing is used
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:74:10: warning: 'begin' violates the C++ One Definition Rule [-Wodr]
void begin(uint8_t address, bool receive_broadcast = 0, uint8_t second_address = 0);
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:182:6: note: implicit this pointer type mismatch
void TwoWire::begin(uint8_t address, bool receive_broadcast, uint8_t second_address) {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:62:7: note: type 'struct TwoWire' itself violates the C++ One Definition Rule
class TwoWire: public Stream {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:182:6: note: 'begin' was previously declared here
void TwoWire::begin(uint8_t address, bool receive_broadcast, uint8_t second_address) {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:182:6: note: code may be misoptimized unless -fno-strict-aliasing is used
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:107:10: warning: 'onReceive' violates the C++ One Definition Rule [-Wodr]
void onReceive(void (*)(int));
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:728:6: note: implicit this pointer type mismatch
* there is just one Wire object, so the code doesn't have to check if Wire is using TWI0 or TWI1
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:62:7: note: type 'struct TwoWire' itself violates the C++ One Definition Rule
class TwoWire: public Stream {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:728:6: note: 'onReceive' was previously declared here
* there is just one Wire object, so the code doesn't have to check if Wire is using TWI0 or TWI1
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:728:6: note: code may be misoptimized unless -fno-strict-aliasing is used
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:108:10: warning: 'onRequest' violates the C++ One Definition Rule [-Wodr]
void onRequest(void (*)(void));
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:742:6: note: implicit this pointer type mismatch
}
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:62:7: note: type 'struct TwoWire' itself violates the C++ One Definition Rule
class TwoWire: public Stream {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:742:6: note: 'onRequest' was previously declared here
}
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:742:6: note: code may be misoptimized unless -fno-strict-aliasing is used
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:84:13: warning: 'endTransmission' violates the C++ One Definition Rule [-Wodr]
uint8_t endTransmission(bool);
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:326:9: note: implicit this pointer type mismatch
uint8_t TwoWire::endTransmission(bool sendStop) {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:62:7: note: type 'struct TwoWire' itself violates the C++ One Definition Rule
class TwoWire: public Stream {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:326:9: note: 'endTransmission' was previously declared here
uint8_t TwoWire::endTransmission(bool sendStop) {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:326:9: note: code may be misoptimized unless -fno-strict-aliasing is used
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:128:18: warning: 'Wire' violates the C++ One Definition Rule [-Wodr]
extern TwoWire Wire;
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:62:7: note: type 'struct TwoWire' itself violates the C++ One Definition Rule
class TwoWire: public Stream {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.cpp:778:11: note: 'Wire' was previously declared here
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/wire.h:85:13: warning: 'endTransmission' violates the C++ One Definition Rule [-Wodr]
uint8_t endTransmission(void) {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:85:13: note: implicit this pointer type mismatch
uint8_t endTransmission(void) {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:62:7: note: type 'struct TwoWire' itself violates the C++ One Definition Rule
class TwoWire: public Stream {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/wire.h:62:7: note: the incompatible type is defined here
class TwoWire: public Stream {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:85:13: note: 'endTransmission' was previously declared here
uint8_t endTransmission(void) {
^
/Users/user/Downloads/Arduino-Port1.18.19-dxc.app/Contents/Java/portable/packages/DxCore/hardware/megaavr/1.4.10/libraries/Wire/src/Wire.h:85:13: note: code may be misoptimized unless -fno-strict-aliasing is usedYou have to change the CPU, compile, and then choose your actual cpu. Arduino bug
ObviousInRetrospect
commented
2 years ago it also seems to be about 8x-10x slower without the check which I don't understand. 7 seconds = 1000 iterations with the slaveTransactionCheck and ~120 without.
here is the code I am testing with https://github.com/ObviousInRetrospect/DualMode/commit/72c6cdd6c134b5d452432b0c9713e4028b5702ca
the bits I am toggling between are: comment/uncomment this line:
#define CHECK_OPENwhen CHECK_OPEN is defined it has the old safe checks. When it isn't sleep_cpu is invoked without regard to slaveTransactionOpen.
and
#define SLP_M SLEEP_MODE_STANDBYyeah changing the CPU fixed the pile of warnings.
I think I actually misdiagnosed it. This time I was careful to do the CPU swap when changing versions, I think it is failing sometimes to rebuild something.
SLEEP_MODE_PWR_DOWN is broken with a nearly 100% failure rate by your latest version regardless of the checks. It works fine in the prior version.
Attaching both: src-new-bad.zip wasgood.zip
ObviousInRetrospect
commented
2 years ago on the other hand, the new version is easier to use in standby. on a third hand doing so is very slow.
MX682X
commented
2 years ago can you also send the .lst/.elf files? Also, please elaborate what you mean by slow
ObviousInRetrospect
commented
2 years ago new code non-working: DualModeExample.ino.lst.zip
old code working: DualModeExample.ino.lst.zip
I am having serious reproducibility issues with the new code. A lot of the time it just doesn't respond at all (which is what the above is doing). When I thought I had it working, if you remember the test that outputs: CRC valid fail: 0 pass: 18022 ...
When the test for slaveModeTransactionOpen was enabled (#define CHECK_OPEN) the iterations counted up at a rate of about 7 seconds per thousand. With that not defined they counted up at a rate of about 120 per 7 seconds.
but the only thing I am sure of is dumping in your August code [edit: wasgood.zip] reliably works and I'm having trouble in various forms with the new code.
ObviousInRetrospect
commented
2 years ago elf files ... do you want me to switch to the DA48 curiosity nano? this was being tested on a custom 64DB32 board.
MX682X
commented
2 years ago yeah
MX682X
commented
2 years ago just checked the lst files. just by the fact that it still uses e.g. SlaveIRQ_AddrWrite means that it's actually the old code
ObviousInRetrospect
commented
2 years ago I'm confused then on why dropping in the contents of src-new-bad.zip vs wasgood.zip is changing whether it works
this is being done in a portable 1.8.19 install to in theory exclude the possibility of it picking up other versions.
ObviousInRetrospect
commented
2 years ago can you check those zips and see whether I am actually testing the code you want?
grep -r SlaveIRQ_AddrWrite .
./twi.c:void SlaveIRQ_AddrWrite(struct twiData *_data);
./twi.c: *@brief SlaveIRQ_AddrWrite is a subroutine of TWI_HandleSlaveIRQ and handles the Address Write case
./twi.c:void SlaveIRQ_AddrWrite(struct twiData *_data) {does show up in the new code.
MX682X
commented
2 years ago It is in fact still there, since it's not the final version, but it is never called. This is the new Handler: As you can see, the sub functions are not called in the new version. (This was copied from your new not working zip) Also, I wasn't able to see the new push/pop sleep functions nor was it using the Y register (which significantly improves speed)
Are you sure you are selecting the correct board? I just realized that the first line is the declaration, then documentation and then function definition. so the grep is not showing any calls.
void TWI_HandleSlaveIRQ(struct twiData *_data) {
__asm__ __volatile__("\n\t": : "y" (_data)); // force _data value into Y register
__asm__ __volatile__("\n\t": "=&y" (_data) : ); // tell compiler that _data is in Y. It does not know the value
// of _data so it won't try to "optimize" the code by using lds/sts. It will use displacement
uint8_t *address, *txHead, *txTail, *txBuffer, *rxHead, *rxTail, *rxBuffer;
#if defined(TWI_MANDS) // Master and Slave split
address = &(_data->_incomingAddress);
txHead = &(_data->_bytesToReadWriteS);
txTail = &(_data->_bytesReadWrittenS);
rxHead = &(_data->_bytesToReadWriteS);
rxTail = &(_data->_bytesReadWrittenS);
txBuffer = _data->_trBufferS;
rxBuffer = _data->_trBufferS;
#else // Slave using the Master buffer
address = &(_data->_clientAddress);
#if defined(TWI_MERGE_BUFFERS) // Same Buffers for tx/rx
txHead = &(_data->_bytesToReadWrite);
txTail = &(_data->_bytesReadWritten);
rxHead = &(_data->_bytesToReadWrite);
rxTail = &(_data->_bytesReadWritten);
txBuffer = _data->_trBuffer;
rxBuffer = _data->_trBuffer;
#else // Separate tx/rx Buffers
txHead = &(_data->_bytesToWrite);
txTail = &(_data->_bytesWritten);
rxHead = &(_data->_bytesToRead);
rxTail = &(_data->_bytesRead);
txBuffer = _data->_txBuffer;
rxBuffer = _data->_rxBuffer;
#endif
#endif
#if defined(TWI_MANDS)
_data->_bools._toggleStreamFn = 0x01;
#endif
uint8_t action = 0;
uint8_t clientStatus = _data->_module->SSTATUS;
if (clientStatus & TWI_APIF_bm) { // Address/Stop Bit set
if (clientStatus & TWI_AP_bm) { // Address bit set
if (clientStatus & TWI_DIR_bm) { // Master is reading
if ((*rxHead) > 0) { // There is no way to identify a REPSTART,
popSleep(); // Workaround: On REPSTART there are two pushes, but one pop otherwise
if (_data->user_onReceive != NULL) { // so when a Master Read occurs after a Master write
_data->user_onReceive((*rxHead)); // issue a call to the user callback first
}
}
(*txHead) = 0; // reset buffer positions so the Master can start writing at zero.
(*txTail) = 0;
(*address) = _data->_module->SDATA; // saving address to expose to the user sketch
if (_data->user_onRequest != NULL) {
_data->user_onRequest();
}
if ((*txHead) == 0) { // If no data to transmit, send NACK
action = TWI_ACKACT_bm | TWI_SCMD_COMPTRANS_gc; // NACK + "Wait for any Start (S/Sr) condition"
} else {
action = TWI_SCMD_RESPONSE_gc; // "Execute Acknowledge Action succeeded by reception of next byte"
}
} else { // Master is writing
(*address) = _data->_module->SDATA;
action = TWI_SCMD_RESPONSE_gc; // "Execute Acknowledge Action succeeded by reception of next byte"
(*rxHead) = 0; // reset buffer positions so the Master can start writing at zero.
(*rxTail) = 0;
}
pushSleep();
} else { // Stop bit set
popSleep();
_data->_module->SSTATUS = TWI_APIF_bm; // Clear Flag, no further action needed
if (_data->user_onReceive != NULL) {
_data->user_onReceive((*rxHead));
}
(*rxHead) = 0;
(*txHead) = 0;
(*rxTail) = 0;
(*txTail) = 0;
}
} else if (clientStatus & TWI_DIF_bm) { // Data bit set
if (clientStatus & TWI_DIR_bm) { // Master is reading
if ((clientStatus & (TWI_COLL_bm | TWI_RXACK_bm)) && // If a collision was detected, or RXACK bit is set AND
(true == _data->_bools._ackMatters)) { // And we have to check for it
(*txHead) = 0; // Abort further data writes
_data->_bools._ackMatters = false; // stop checking for NACK
action = TWI_SCMD_COMPTRANS_gc; // "Wait for any Start (S/Sr) condition"
} else { // RXACK bit not set, no COLL
_data->_bytesTransmittedS++; // increment bytes transmitted counter (for register model)
_data->_bools._ackMatters = true; // start checking for NACK
if ((*txTail) < (*txHead)) { // Data is available
_data->_module->SDATA = txBuffer[(*txTail)]; // Writing to the register to send data
(*txTail)++; // Increment counter for sent bytes
action = TWI_SCMD_RESPONSE_gc; // "Execute a byte read operation followed by Acknowledge Action"
} else { // No more data available
action = TWI_SCMD_COMPTRANS_gc; // "Wait for any Start (S/Sr) condition"
}
}
} else { // Master is writing
rxBuffer[(*rxHead)] = _data->_module->SDATA; // reading SDATA will clear the DATA IRQ flag
if ((*rxHead) < (BUFFER_LENGTH - 1)) { // if buffer is not yet full
(*rxHead)++; // Advance Head
action = TWI_SCMD_RESPONSE_gc; // "Execute Acknowledge Action succeeded by reception of next byte"
} else { // else buffer would overflow with next byte
action = TWI_ACKACT_bm | TWI_SCMD_COMPTRANS_gc; // "Execute ACK Action succeeded by waiting for any Start (S/Sr) condition"
}
}
}
_data->_module->SCTRLB = action; // putting this at the end reduces the loads of _module pointer
#if defined(TWI_MANDS)
_data->_bools._toggleStreamFn = 0x01;
#endif
}Nevermind, I forgot to check the .lst size. It's two times the same file
ObviousInRetrospect
commented
2 years ago oy I was looking in the 2.0 build path and didn't notice I gave you those files and not the files 1.8.19 was building
ObviousInRetrospect
commented
2 years ago these are actually built against what I said they are instead of being residual garbage unaffected by compilation or changing libraries.
MX682X
commented
2 years ago I found the problem. I decided that it might have been a good idea to notify the sketch if a Master write with 0 written bytes happend. Like: Master has looked for you. This resulted in void receiveHandler(int numbytes) being called with numbytes = 0. and you were doing numbytes -= 2; while (numbytes) which lead to a long loop slowing down the whole thing. I've checked the Arduino API and they call the Handler only if there were bytes written, so I added this check to the isr now.
RC2: src.zip
P.S.: I recommend you to add checks on your Wire.endTransmission and Wire.requestFrom(in the client code) to easier understand where an error comes from, like Timeout, or ADR/DATA NACKs.
SpenceKonde
commented
2 years ago This thread is a real slog. Is the code in megatinycore also impacted. and hence needs update from this?
MX682X
commented
2 years ago Yes, this also affects megaTinyCore. The current version is working, I was just unhappy how complicated it was to make sure you could use TWI in Power Down so I tried to improve it. Also incoperates a couple of other improvements in form of reduced Flash usage (e.g. use VPORT instead of PORT to clear pins). Now the user doesn't have to think of when they are allowed to execute sleep, library takes care of that. (technically my popSleep and pushSleep functions can be used by up to 15 peripherals at the same time with just one byte RAM usage - like oh, UART has still bytes left to write? we only allow IDLE untill we're done). Basically intended for 2.6.2 / 1.5.1
SpenceKonde
commented
2 years ago Can you describe the concept push/popping sleep?
MX682X
commented
2 years ago static uint8_t sleepStack = 0;
void pushSleep() {
#if defined(TWI_USING_WIRE1)
uint8_t sleepStackLoc = sleepStack;
if (sleepStackLoc > 0) { // Increment only if sleep was enabled
sleepStackLoc = (sleepStackLoc + 0x10); // use upper nibble to count - max 15 pushes
} else {
sleepStackLoc = SLPCTRL.CTRLA; // save sleep settings to sleepStack
SLPCTRL.CTRLA = sleepStackLoc & 0x01; // Set to IDLE if sleep was enabled
} // If
sleepStack = sleepStackLoc;
#else
sleepStack = SLPCTRL.CTRLA; // save old sleep State
SLPCTRL.CTRLA = sleepStack & 0x01; // only leave the SEN bit, if it was set
#endif
}
void popSleep() {
#if defined(TWI_USING_WIRE1)
uint8_t sleepStackLoc = sleepStack;
if (sleepStackLoc > 0) { // only do something if sleep was enabled
if (sleepStackLoc > 0x10) { // only decrement if pushed once before
sleepStackLoc = (sleepStackLoc - 0x10); // upper nibble
} else { // at 0 we are about to put sleep back
SLPCTRL.CTRLA = sleepStackLoc; // restore sleep
sleepStackLoc = 0; // reset everything
}
sleepStack = sleepStackLoc;
}
#else
SLPCTRL.CTRLA = sleepStack;
#endif
}lower nibble is used to store the sleep settings, upper nibble is used to store the number of calls to push and pop. First time called push: Copies sleep settings and only leaves sleep enabled in the CTRLA register. then the counter can be incremented up to 15 times until it overflows and everything breaks. But this can be covered by limiting the places where it is called. As long it's not used by accident somewhere in the sketch (gonna blame the user in that case) In the core itself we have like 5 USARTS, 2 TWI and 2SPI top, less then 10, so no problem. When called pop, the number decrements, until it reaches 0, then the sleep settings are copied back into CTRLA.
(if used only in one spot/ISR, this can be made extremely simpler, as you can see with the #if defined )
ObviousInRetrospect
commented
2 years ago There might be a slight problem with this approach:
If sleep and wake are frequent what you describe is wonderful and a huge usability improvement. On my board that wakes at 16hz but currently needs to juggle between sleep modes depending on whether it has the GPS on and needs serial to not break I totally love the concept of the TWI and UART drivers taking the edge cases out of my code for choosing the right sleep mode until done. Especially since those edge cases, to be done correctly, are pretty ugly to get the synchronization right.
On the other hand if a device sometimes (thinks it) goes into sleep_mode_pwr_down waiting on a push button (think a coin cell remote) going into sleep_mode_idle for days could be disastrous.
Not sure if there is some solution around re-sleeping on the wake that causes the pop or if it is just a documentation issue. But if its a documentation issue I think the default should be push/pop and a boards menu entry to turn off automatic sleep hygiene.
(working on getting tests up and running for the new code. yeah I never thought to handle the case where no bytes were sent. I am probably not the only one)
ObviousInRetrospect
commented
2 years ago new version is working in sleep mode power down without any regard to wire in its use of sleep:
while(!wake){
Serial.flush();
sleep_cpu();
}
#endifCRC valid
fail: 0 pass: 26598will let it run for at least a few hours but 25K catches a lot of stuff
happen to be testing on a board with an ina3221 handy where one of the channels is the board's power use so going to dig in a bit to make sure the new code spends most of its sleep time in the intended mode.
SpenceKonde
commented
2 years ago You are a hero! (both of you)
MX682X
commented
2 years ago On the other hand if a device sometimes (thinks it) goes into sleep_mode_pwr_down waiting on a push button (think a coin cell remote) going into sleep_mode_idle for days could be disastrous.
I cannot imagine someone would risk going into sleep while USART is not finished transmitting (TWI was broken before, not counting that), so people would normally wait for USART to finish - by using flush. Since flush waits for buffer empty, this change shouldn't affect old code, but might make life easier for new code. Consider this: Now, a device that operates on a coin cell, where the code calls sleep() in a loop if nothing has to be done, can save about 20% of energy (see graph above) while waiting for a transmission to complete, by automatically going into IDLE, and then to POWER DOWN when the transmission is finished.
for clarification: the push and pop of sleep happens in the ISR, on end (e.g. STOP) and beginning (e,g. START) respectively.
ObviousInRetrospect
commented
2 years ago yeah I guess the question is whether the terminal sleep could happen while it was still sleeping lightly but I can't come up with a case other than a twi transaction that is cut off part way through.
meanwhile, over 1 million, almost certainly fine. will let it run until I need the serial port. This is the RC2 code running on DXCore using sleep_mode_pwr_down and no guard. Which is way better than the correct guard (inline assembly for the sei/sleep for those of us who don't trust gcc not to reorder something).
CRC valid
fail: 0 pass: 1050050actually kind of tempted to stick the new code into MTC 2.6.1 and make the master run that.
I've noticed unreliable slave behavior in SLEEP_MODE_STANDBY (and SLEEP_MODE_POWER_DOWN). This seems to be more of an issue when transferring larger buffers. I thought guarding the sleep_cpu() call with !Wire.slaveTransactionOpen() was sufficient. The twi supports wake from STANDBY and even POWER_DOWN but every i2c slave I have written with DxCore and megaTinyCore has been flakey unless set for SLEEP_MODE_IDLE.
The code here: https://github.com/ObviousInRetrospect/DualMode/tree/main/DualModeExample
works as expected
but if SLEEP_MODE_IDLE is changed to SLEEP_MODE_STANDBY the slave starts corrupting data: https://github.com/ObviousInRetrospect/DualMode/blob/10dfca6b5bc6c8fedf1605d2d8c5981e2e4aca34/DualModeExample/DualModeExample.ino#L319
this is most easily seen (especially without an ina3221) by uncommenting the test pattern data fill (L184-188): https://github.com/ObviousInRetrospect/DualMode/blob/10dfca6b5bc6c8fedf1605d2d8c5981e2e4aca34/DualModeExample/DualModeExample.ino#L184
When I raised this in https://github.com/SpenceKonde/DxCore/discussions/316 @MX682X suggested opening a separate issue with more details.
The output on the master https://github.com/ObviousInRetrospect/DualMode/tree/main/DualModeExampleClient in SLEEP_MODE_IDLE is a long string of correct reads:
(the master does a 32-byte read follow by a 25-byte read. the last 4 bytes are a crc32 covering the entire data)
omitting thousands of similar lines with very few errors. In an overnight run ran this overnight, 99.2% of transfers were clean, 0.8% corruption due to race condition around crc calculation where data looks fine but the data changed during crc calculation.
when the slave is changed to SLEEP_MODE_STANDBY, the output changes to:
[content in square brackets are comments I added]
This is with test pattern data in which each byte contains its own address