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.

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

Giga: compatibility fixes with M4 builds #741

Merged
merged 10 commits into from
Nov 13, 2023
Merged

Conversation

facchinm
Copy link
Member

Fixes #726 and supersedes #740

@KurtE
Copy link

KurtE commented Oct 24, 2023

@facchinm

Not sure if it is best to ask this here or on forum or new issue?

In a test sketch, that I am using to test my version of ILI9341 library, I added Serial1.begin(1000000);
as well as some Serial1.print or write...

On M7 works fine.
On M4 it outputs at about 640KB

Thanks
Kurt

@facchinm
Copy link
Member Author

@KurtE using this PR or without? I just tested with this PR applied and the baud is correct

@KurtE
Copy link

KurtE commented Oct 24, 2023

Thanks @facchinm
Keeping my fingers crossed, maybe it is the issue of not being able to use bluetooth...
#723

@AndrewCapon
Copy link

AndrewCapon commented Oct 25, 2023

Hi @facchinm

I have done a bit of testing with the fix and both the M4 and M7 now agree on microseconds but not on milliseconds, so I'm guessing the M7 may have an issue with milliseconds?.

Using some test code here with the M4 using delay(1000) over 120 seconds has the following drift from M7 to M4:
ms = -2779 ms, us = 219 us.

Using some test code here with the M4 spinning on micros() over 120 seconds has the following drift from M7 to M4:
ms = -2796 ms, us = 1234 us.

The us figures seem good but the ms figures are quite a bit off.

M4 code

#include <RPC.h>
uint32_t uFreq;
unsigned long uTotalMiliStart;
unsigned long uTotalMicroStart;

void setup() {
  RPC.begin();
  uTotalMiliStart = millis();
  uTotalMicroStart = micros();

  uFreq = HAL_RCC_GetSysClockFreq();
}

bool bUseDelay = false;
uint32_t uLoopCount = 0;

void loop() 
{
  if(uLoopCount < 120)
  {
    unsigned long uMiliStart = millis();
    unsigned long uMicroStart = micros();
    unsigned long uEnd = uMicroStart + 1000000;

    if(bUseDelay)
    {
      delay(1000);
    }
    else
    {
      while(micros() < uEnd)
        ;
    }

    unsigned long uMiliEnd = millis();
    unsigned long uMicroEnd = micros();

    char buffer[2048];
    sprintf(buffer, "M4 (%lu, %s, %u) %lu, %lu, %lu, %lu", uLoopCount, bUseDelay ? "Delay" : "Spin", uFreq, 
      uMiliEnd - uMiliStart, uMicroEnd - uMicroStart, 
      uMiliEnd - uTotalMiliStart, uMicroEnd - uTotalMicroStart);

    RPC.println(buffer);
    uLoopCount ++;
  }
}

M7 code

#include <RPC.h>

uint32_t uFreq;
unsigned long uMiliStart;
unsigned long uMicroStart;
unsigned long uTotalMiliStart;
unsigned long uTotalMicroStart;

void setup() {
  // put your setup code here, to run once:
  Serial.begin(9600);
  while(Serial.available() == 0)
    ;

  RPC.begin();
  uTotalMiliStart = millis();
  uTotalMicroStart = micros();

  uFreq = HAL_RCC_GetSysClockFreq();
  
}


void loop() 
{
  //Serial.println("loop");
  String buffer = "";
  while (RPC.available()) {
    buffer += (char)RPC.read();
  }

  if (buffer.length() > 0) {
    unsigned long uMiliEnd = millis();
    unsigned long uMicroEnd = micros();

    Serial.print(buffer);

    char m7buffer[1024];
    sprintf(m7buffer, "M7 (%lu) %lu, %lu, %lu, %lu\n", uFreq, 
      uMiliEnd - uMiliStart, uMicroEnd - uMicroStart,
      uMiliEnd - uTotalMiliStart, uMicroEnd - uTotalMicroStart);

    Serial.println(m7buffer);

    uMiliStart = millis();
    uMicroStart = micros();

  }
}

some data:

With fix and using delay(1000)

M4 (0, Delay, 480000000)  1000,   996638,   1000,   996644
M7 (480000000)            16599,  16618106, 977,    997604

M4 (1, Delay, 480000000)  1000,   996758,   2000,   1993658
M7 (480000000)            977,    997393,   1954,   1995171

...

M4 (118, Delay, 480000000) 1000,  996696,   119000, 118639208
M7 (480000000)              977,  997308,   116212, 118640952

M4 (119, Delay, 480000000) 1000,  996647,   120000, 119636118
M7 (480000000)              976,  996166,   117188, 119637297

So 1000ms delay on the M4 seems to be around 977 ms on the M7
While the same 1000ms delay has roughly the same microseconds on both.

The cumulative error over 120 seconds

-33 ms different to -2812 ms = - 2779 ms
960 us different to 1179 us  = 219 us

So both cores are agreeing on us but not ms.

With fix and spinning on us 

M4 (0, Spin, 480000000)     1003,   1000006,  1003,   1000012
M7 (480000000)              65829,  65842288, 980,    1001065

M4 (1, Spin, 480000000)     1003,   1000007,  2006,   2000271
M7 (480000000)              980,    1000559,  1960,   2001822

...

M4 (118, Spin, 480000000)   1002,   1000006,  119306, 119031244
M7 (480000000)              979,    999991,   116511, 119032517

M4 (119, Spin, 480000000)   1002,   1000007,  120309, 120031520
M7 (480000000)              979,    1000084,  117490, 120032754

So spinning on us, 

so 1000000 us on the M4 is nearly 1000 ms but on the m7 is around 980.
microseconds are again good.

The cumulative error over 120 seconds

-23 ms different to -2819 ms = -2796 ms
1053 us different to 1179 us = 1234 us

So same again, us basically right, ms is wonky.

@AndrewCapon
Copy link

AndrewCapon commented Oct 25, 2023

I put a logic analyser to work.

microseconds are spot on for each.

milliseconds are wrong and different for each, one slow, one fast.

The issue seems to be the mbed::LowPowerTimer used for ms, if you undefine DEVICE_LPTICKER then ms are also spot on for each.

This allows having all the generic code in a single place, including the needed clock configuration override
@Nocentinia
Copy link

Sorry, I'm not very familiar with GitHub.. so does it work now?

@KurtE
Copy link

KurtE commented Oct 26, 2023

@Nocentinia - It is not merged yet.

Once it is merged, so it is not yet in any build. But I am keeping my fingers crossed that it will be soon

@AndrewCapon
Copy link

Hi @facchinm

As you are looking into the M4 I have found out that reading from the SDRAM is not working correctly on this core.

https://forum.arduino.cc/t/sdram-read-not-working-correctly-on-m4/1182865

Cheers

Andy

@facchinm
Copy link
Member Author

@AndrewCapon the last commit of this series 3490221 fixes the issue of using the SDRAM on M4; however, the RAM cannot be initialized by both M7 and M4 but this is currently not guarded.

@AndrewCapon
Copy link

Great @facchinm thanks for that :)

@trylaarsdam
Copy link
Contributor

trylaarsdam commented Oct 30, 2023

@facchinm could you confirm the expected behavior of WiFi on the M4 core after these changes? I see that you removed the define for the M4 in the wl_definitions file but didn't add one for the generic M4 core. Does this officially mean that the M4 should not/cannot manage the WiFi portion of the Murata radio on the Portenta?

Currently the behavior on the public release of the core is that calling WiFi.begin(ssid, psk); from the M4 core just hangs forever. Thanks for your work on BLE - it's fantastic to know the M4 is being worked on.

Edit: I should also add I pulled your forks of the Mbed core and BLE library and was able to compile them and the Portenta no longer panics when the M4 core calls BLE.begin(), but it hangs and does not continue with execution. I know it's still in progress but just wanted to add what I was experiencing with the current commits.

@facchinm
Copy link
Member Author

facchinm commented Nov 3, 2023

@trylaarsdam I just retested the BLE patchset from scratch and everything seem to work on my side, however I'll provide some more "finished" examples as soon as the patchset gets merged.

Talking about wifi, instead, there seem to be some hardware idiosyncrasies between the M4 and SDMMC1 bus, no documentation from ST states that it's impossible so I'll still try for some days to get it working. If nothing happens, I'm going to merge this PR as is so we can at least have basic HW functionalities on the M4

@facchinm facchinm merged commit 9cb7ce5 into arduino:main Nov 13, 2023
10 checks passed
@facchinm facchinm mentioned this pull request Nov 13, 2023
@Bexin3
Copy link

Bexin3 commented Nov 23, 2023

Well it looks like this likely made my code no longer compatible with M4

@facchinm
Copy link
Member Author

@Bexin3 can you expand? Or share the code that doesn't work anymore? The change was made to be completely backwards compatible so any regression will be investigated seriously.

@Bexin3
Copy link

Bexin3 commented Nov 24, 2023

Hello, the code can be found here;
https://forum.arduino.cc/t/a-code-that-lets-you-zoom-into-rotate-and-move-a-camera-feed-on-giga-r1/1192843
The M4 core can no longer read the camera, as I use it for offloading as DMA isn't implemented.
Further, I can still see pretty bad screen tear for some reason.

@Bexin3
Copy link

Bexin3 commented Nov 24, 2023

The screen tear only happens when the image changes though

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 this pull request may close these issues.

Timing issue on GIGA on M4
6 participants