search

sparkfun / OpenLog_Artemis

The OpenLog Artemis is an open source datalogger the comes preprogrammed to automatically log IMU, GPS, serial data, and various pressure, humidity, and distance sensors. All without writing a single line of code!
https://www.sparkfun.com/products/15846
Other
88 stars 47 forks source link

OLA waking up from sleep and going into menu when there's a signal on the RX pin. #117

Closed ryanneve closed 2 years ago

ryanneve commented 2 years ago

Subject of the issue

Despite the firmware change we pushed, we continue to sometimes see OLA waking up when it shouldn't. Note that this isn't always reproducible, but when it gets in this state, it occurs continuously.

Your workbench

We have noted that after power is removed from qwiic bus, signal on BREAKOUT_PIN_RX drops slowly from 3v3 to 0. Can provide oscilloscope image if it helps, but I thought the new am_hal_gpio_pinconfig(13 , g_AM_HAL_GPIO_DISABLE); //RX1 before sleeping would make the OLA ignore that pin.

Steps to reproduce

If we disconnect wire from OLA RX to BLE TX, problem doesn't occur.

Expected behavior

OLA should stay asleep until woken by timer.

Actual behavior

Sample is taken. OLA goes to sleep (as verified in debug statements). OLA wakes up and goes into menu.

Is there a way to tell what interrupt woke the OLA?

OLA_settings.txt OLA_deviceSettings.txt

PaulZC commented 2 years ago

Hi Ryan,

I’m wondering if the serial Rx interrupt is still enabled, even though the pin is disabled? The code for this will be buried in the Apollo3 core and won’t be easy to find…

I wonder if a logic buffer will solve your issue? A simple non-inverting buffer that goes high impedance when the power goes off should do the trick? Let me know if you need some suggestions.

All the best, Paul

ryanneve commented 2 years ago

Paul,

I'm going to try turning power off to the qwiic bus earlier in the goToSleep() sequence with the hope that any signal from the attached BLE module will be done by the time we go into deep sleep.

The configureSerial1TxRx() function in OpenLog_Artemis.ino sets a pull-up on the RX pin. Should this be disabled before Deep Sleep? This might allow the signal on that pin to drop to ground faster. Not sure of the syntax to do this. am_hal_gpio_pincfg_t (g_AM_BSP_GPIO_COM_UART_RX.ePullup, AM_HAL_GPIO_PIN_PULLUP_NONE); It looks to already be re-enabled in wakeFromSleep()

As far as digging into the Apollo3 core. Perhaps Serial1.end() may not be detaching the interrupts.

I looked some at Arduino_Apollo3, but couldn't find much related to Serial or Serial interrupts. There is this issue of yours https://github.com/sparkfun/Arduino_Apollo3/issues/423 which may relate.

I also looked in ARMmbed/mbed-os where I found that serial_api.c has a serial_irq_set function which enables and disables serial interrupts using am_hal_uart_interrupt_enable() and am_hal_uart_interrupt_disable(). It looks like serial_irq_set() is called from the attach() method in SerialBase.cpp. I can't find where to look to see if Serial1.end() detaches its interrupts.

paulvha commented 2 years ago

I saw a link to this issue in a post I did earlier on the Artemis library issue. I had a quick look at this.

The issue is related to that in V2.x.x Serial1.end(), defined in HardwareSerial.cpp, does NOTHING. It is an empty function. The same is true for flush(), also an empty function.

In V1.2.3 Serial1.end does perform real activity in power-down and thus disable the UART

 if (_handle != NULL)
    {
        flush();

        // Power down the UART, and surrender the handle.
        am_hal_uart_power_control(_handle, AM_HAL_SYSCTRL_DEEPSLEEP, false);
        am_hal_uart_deinitialize(_handle);

        // Disable the UART pins.
        am_hal_gpio_pinconfig(_pinTX, g_AM_HAL_GPIO_DISABLE);
        am_hal_gpio_pinconfig(_pinRX, g_AM_HAL_GPIO_DISABLE);

        _handle = NULL;
    }

and flush () on V1

    // Make sure the UART has finished sending everything it's going to send.
    am_hal_uart_tx_flush(_handle);

There is an MBED function mbed::UnbufferedSerial::_deinit(), but that is defined as private and can not be called from Serial1.end(). If however it could be called, in MBED that is passed on to Serial_free() in serial.api.c (part of target_Apollo3). In this file, that function is not doing anything either.

void serial_free(serial_t *obj) { 
  // nothing to do unless resources are allocated for members of the serial_s serial member of obj
  // assuming mbed handles obj and its members
}

In order to change this correctly, there needs to be a change in a number of places to enable a proper deepsleep and recovery from sleep.

@PaulZC: this could be one of the reasons why people measure a higher current in deepsleep on V2. This issue relates to both Serial and Serial1. I think you need to discuss this with Kyle. He can always contact me and in the meantime, I'll try- just for the fun - to make my own hacked version.

regards Paulvha

paulvha commented 2 years ago

here are the changes needed to make it happen : ARDUINO changes :

In HardwareSerial.h, add around line 19 :

    PinName _pinTX;      
    PinName _pinRX;

In HardWareSerial.cpp, add around line 19 in begin():

    _pinTX = tx;
    _pinRX = rx;

add around line 137 in end():


    void UART::end( void ){

        UnbufferedSerial::_deinit();  // powerdown and de-initialize

        // Disable the UART pins.
        am_hal_gpio_pinconfig(_pinTX, g_AM_HAL_GPIO_DISABLE);
        am_hal_gpio_pinconfig(_pinRX, g_AM_HAL_GPIO_DISABLE);
    }

MBED changes:

In SerialBase.h move __deinit() from line 305 to 146 (private to public)

In UnbufferedSerial.h add around line 163: using SerialBase::_deinit;

In serial.api.c. (part of TARGET_Apollo3). This needs a recompile for the archive stored in variants/mbed.

change in serial_free() from :

    void serial_free(serial_t *obj) {
       // nothing to do unless resources are allocated for members of the serial_s serial member of obj
       // assuming mbed handles obj and its members
    }

to :


    void serial_free(serial_t *obj) {

        // Power down the UART, and surrender the handle.
        am_hal_uart_power_control(obj->serial.uart_control->handle, AM_HAL_SYSCTRL_DEEPSLEEP, false);
        am_hal_uart_deinitialize(obj->serial.uart_control->handle);
    }

It is possible. with little effort.

regards, paulvha

PaulZC commented 2 years ago

Hi Paul (@paulvha ),

Sincere thanks for this as always - it helps a lot!

Ryan (@ryanneve ): I will try and compile an updated OLA binary for you including these changes. I'm buried in other projects at the moment, but I will get to it as soon as I can. For speed, I might just include Paul's changes directly into the OLA code. Longer term, this needs fixing properly as Paul suggests.

Best wishes, The other Paul

paulvha commented 2 years ago

detected one glitch this morning that I am working on right now. so there will be an update.

paulvha commented 2 years ago

so this glitch turned out to be a harder one to diagnose, but found the root cause and solution. On THIS Location if have posted the following information:

look in the README.md file how to install. Look forward to the feedback.

regards, Paulvha

PaulZC commented 2 years ago

Hi Paul (@paulvha ),

It has taken a very long time, but I am now trying your fix for this issue. (Thank you again for sharing it!)

I am seeing some strange compilation errors:

C:\Users\pc235\AppData\Local\Arduino15\packages\SparkFun\hardware\apollo3\2.2.0\cores\arduino\mbed-bridge\core-implement\HardwareSerial.cpp: In member function 'virtual void UART::end()':
C:\Users\pc235\AppData\Local\Arduino15\packages\SparkFun\hardware\apollo3\2.2.0\cores\arduino\mbed-bridge\core-implement\HardwareSerial.cpp:128:31: error: 'void mbed::SerialBase::_deinit()' is inaccessible within this context
     UnbufferedSerial::_deinit();  // power down - edit pvh
                               ^
In file included from C:\Users\pc235\AppData\Local\Arduino15\packages\SparkFun\hardware\apollo3\2.2.0/cores/mbed-os/drivers/UnbufferedSerial.h:26,
                 from C:\Users\pc235\AppData\Local\Arduino15\packages\SparkFun\hardware\apollo3\2.2.0/cores/mbed-os/mbed.h:70,
                 from C:\Users\pc235\AppData\Local\Arduino15\packages\SparkFun\hardware\apollo3\2.2.0/cores/arduino/mbed-bridge/Arduino.h:14,
                 from C:\Users\pc235\AppData\Local\Arduino15\packages\SparkFun\hardware\apollo3\2.2.0/cores/arduino/sdk/ArduinoSDK.h:9,
                 from <command-line>:
C:\Users\pc235\AppData\Local\Arduino15\packages\SparkFun\hardware\apollo3\2.2.0/cores/mbed-os/drivers/SerialBase.h:146:10: note: declared here
     void _deinit(); // paulvha
          ^~~~~~~
C:\Users\pc235\AppData\Local\Arduino15\packages\SparkFun\hardware\apollo3\2.2.0\cores\arduino\mbed-bridge\core-implement\HardwareSerial.cpp:128:31: error: 'mbed::SerialBase' is not an accessible base of 'UART'
     UnbufferedSerial::_deinit();  // power down - edit pvh

I think I must have missed a step...? But I cannot figure it out. Have I done something silly?

I'm using a fresh install of v2.2.0 and have replaced the three files with the ones from your OneDrive.

Sincere thanks, Paul

paulvha commented 2 years ago

wow Paul.. that is lightyears and at least 20 other projects ago :-) :-)

I will have time tomorrow afternoon to look at this. I expect the issue might be related to 

In mbed-os/drivers/Serialbase.h (1 change)

Make _deinit() a public-call instead of private.

Move: void _deinit(); from line 305 to line 146 in the public area

That said.. I have seen a structure in https://github.com/sparkfun/Arduino_Apollo3/issues/454 for Wire, where basics could be applied here to make the changes even simpler.

adamgarbo commented 2 years ago

You know you're in good hands when both of the Pauls are on the case! :stuck_out_tongue_winking_eye:

Very keen to see the outcome of this issue, especially if it addresses the higher quiescent draw that's been observed on v2.x.

paulvha commented 2 years ago

EDIT : I thought I had found a better way, but not yet. The Initialisation of the Apollo3 UART module is done as soon as the UART constructor is created and not during begin(). I found a way to get the MBED _serial pointer in Serial.end(). Then call Serial_free() in which I had included code in there to close down the UART completely. BUT as the next call to Serial.begin() will not initialize the UART module again, the easy setup did not work. I am now trying to see whether I can get it changed like with SPI and Wire-constructor.

My bad, I forgot to include a change. I have updated the document on the share. In mbed-os/drivers/UnbufferedSerial.h (1 change)

Around line 158 add

using SerialBase::_deinit;

To the list : using SerialBase::readable; using SerialBase::writeable; using SerialBase::format; using SerialBase::attach; using SerialBase::baud; using SerialBase::RxIrq; using SerialBase::TxIrq; using SerialBase::IrqCnt;

PaulZC commented 2 years ago

Hi Paul (@paulvha ),

Sincere thanks for this. As my American colleagues would say: "You're the man!"

The fix is working nicely. Activity on the RX pin no longer wakes the Artemis from sleep.

I've got more issues to fix, SPI stops working after coming out of sleep, but I'll investigate and fix that. I suspect I'm not re-enabling something correctly.

Thanks again, Paul

paulvha commented 2 years ago

Good to hear..
With SPI end there were 2 issues: wrong check on dev (https://github.com/sparkfun/Arduino_Apollo3/issues/442) and next to that it should do dev = 0 in end(), else it will not get a new SPI instance during begin()

Still try to get an alternative working for UART, although that is becoming more complex than the current fix. :-) regards Paulvha

PaulZC commented 2 years ago

OK. I think I've got a working version of OLA which fixes this issue. BUT, to get it working, I've had to regress to v2.1.0 of the core...

Why? Well, with v2.2.0 of the core, it looks like SdFat behaves differently and can leave the CE pin low. This causes all kinds of badness when trying to communicate with the IMU (which shares the SPI bus).

Just to prove I'm not making this up:

Here is the OLA start-up with v2.1.0 of the core (modified with the Serial _deinit fix):

2 1 0 full paulvha

At 1000ms, there is a burst of activity on the microSD card which lasts for about 270ms. Then the code pauses for about 280ms. Then you can see three bursts of activity while the code reads the settings from the SD card and opens the log files. Then you can see the code initializing the IMU.

Compare this with v2.2.0 of the core (modified with the Serial _deinit fix):

2 2 0 full paulvha

Notice how the SD card activity is the same BUT the SD CE pin goes low and stays low after the log files are opened. This of course causes all kinds of badness for the IMU and it does not start up correctly.

If I let the code continue, the SD CE stays low until the first file write happens. After that, it seems to recover and the CE appears to function normally, returning to the high state once the activity has finished.

I don't (yet) have a stripped-down example which isolates this. My theory - at the moment - is that something changed in the way SPI operates in v2.2.0, resulting in the strange behavior on the CE pin. Or maybe it is the way digitalWrites or pinModes happen? The CE pin is controlled by SdFat (I'm using v2.1.2 - the latest version). Further diagnosis probably requires digging into SdFat to see how it controls the CE pin and how that could be different with v2.2.0 of the core?

Anyway... I'm going to finish up testing this v2.1.0 hybrid and will hopefully be able to release it in a day or two.

Sincere thanks for all the help @paulvha !

Very best wishes, The other Paul

PaulZC commented 2 years ago

Forgot to mention: both tests included the SPI.end changes too:

        void arduino::MbedSPI::end() {
            if (dev) {
                delete dev;
                dev = NULL;
            }
        }
paulvha commented 2 years ago

Recently I wrote a filemanager (based on SdFat) to read a MicroSD card on different Artemis-based boards (https://github.com/paulvha/apollo3/tree/master/Artemis_filemanager). Trying to understand the SdFat code structure was a challenge, but it is an interesting library. Most routines of SdSpiCard.cpp have the same standard structure: 

Do something
Was it good, spiStop(); and return true
Did it fail, spiStop(); and return false

In spiStop(); it will unselect the CE.

But only a few (and this one most noticeable differ) as it seems to be missing spiStop(); when it fails.

bool SharedSpiCard::readSectors(uint32_t sector, uint8_t* dst, size_t ns) { 
  if (!readStart(sector)) {
    goto fail;
  }
  for (size_t i = 0; i < ns; i++, dst += 512) {
    if (!readData(dst, 512)) {
      goto fail;
    }
  }
  return readStop();
 fail:
  return false;
}

readStop() will send a CMD12 and follow the same standard structure as mentioned earlier.

So maybe the issue is related to an error during reading a sector and this missing spiStop() in the code.

regards, Paulvha

paulvha commented 2 years ago

As for UART power control, I have an much easier to implement solution.

While I was able to apply to UART a similar structure as with SPi and WIRE: creating the link/instance to Mbed during begin() instead of when the constructor is created. It turned out that no matter what did and tried, deleting that instance in UART:end() would cause the sketch to immediately crash. I decided not to delete it and created a working solution that have documented on: https://1drv.ms/u/s!AvgMkLxXj95qhtVHPgi3qegmZR_Xhg?e=XKYMRN

Then based on the learnings of both earlier solutions, there is now a much easier solution. It will require only a few changes. Documented on https://1drv.ms/u/s!AvgMkLxXj95qhtVNhdQgRc5AqJcxeA?e=5Bww8W

The relevant files are also in the locations with the Mbed library compiled with changed serial_free() in serial_api.c for an ATP. regards, Paulvha

PaulZC commented 2 years ago

Thank you Paul - I will give it a try.

Are your changes based on v2.2.0 or v2.2.1?

Have a nice weekend, Paul

paulvha commented 2 years ago

I have used V2.2.1.

regards Paul

PaulZC commented 2 years ago

Hi Ryan (@ryanneve ),

We have a solution for you. Please try version 2.2 of the firmware. It will be released in a few minutes.

Just for reference:

v2.2 is based on v2.2.1 of the Apollo3 core. That's the latest release which went live a few days ago. It includes @paulvha 's UART:end() modifications. I've tested this on V10 and X04 hardware and it works well. Previously, any activity on the RX pin would have woken the Artemis from deep sleep. The RX pin slowly being pulled low as your BLE module discharged would have done the same thing. Paul's fix prevents this.

I have tested the firmware thoroughly, but it has so many options I might have missed something. Please let me know if you see any weirdness.

Hope all is well in Florida - and that you enjoy the new firmware!

Best wishes, Paul

paulvha commented 2 years ago

PaulZC : will you pass this fix on to Wenn0101/ Kyle so it becomes part of the next release of the Artemis library ?

PaulZC commented 2 years ago

Sure - not a problem - Kyle ( @Wenn0101 ) is already aware of the changes - thank you!

ryanneve commented 2 years ago

Thanks! I'll try to get this out in the field (North Carolina) and see how it goes.

PaulZC commented 2 years ago

Hi Ryan,

Oops! Sorry! Senile moment. Indeed, I hope all is well in NC.

Very best wishes, Paul