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: SdFat Bench example fails to load and run correctly #24

Open
mjs513 opened this issue Dec 26, 2024 · 15 comments
Open

GIGA: SdFat Bench example fails to load and run correctly #24

mjs513 opened this issue Dec 26, 2024 · 15 comments

Comments

@mjs513
Copy link

mjs513 commented Dec 26, 2024

Describe the bug
Using the SdFat Bench example the sketch compiles and seems to load however it hangs the sketch.

Output of Serial Monitor

sketch
[00:00:15.652,000] �[1;31m<err> llext: Undefined symbol with no entry in symbol table sbrk, offset 2360, link section 12�[0m
[00:00:15.652,000] �[1;31m<err> llext: Failed to link, ret -61�[0m
Failed to load sketch, rc -61
�[1;32muart:~$ �[m

Tried adding the symbol to llext_exports.c however I receive a build error on sbrk:

In file included from /home/my_new_zephyr_folder/ArduinoCore-zephyr/loader/llext_exports.c:3:
/home/my_new_zephyr_folder/ArduinoCore-zephyr/loader/llext_exports.c:28:15: error: 'sbrk' undeclared here (not in a function)
   28 | EXPORT_SYMBOL(sbrk);
      |               ^~~~
/home/my_new_zephyr_folder/zephyr/include/zephyr/llext/symbol.h:136:62: note: in definition of macro 'Z_EXPORT_SYMBOL'
  136 |                 .name = STRINGIFY(x), .addr = (const void *)&x,                 \
      |                                                              ^
/home/my_new_zephyr_folder/ArduinoCore-zephyr/loader/llext_exports.c:28:1: note: in expansion of macro 'EXPORT_SYMBOL'
   28 | EXPORT_SYMBOL(sbrk);
      | ^~~~~~~~~~~~~
[34/238] Building C object zephyr/CMakeFiles/zephyr.dir/subsys/logging/log_core.c.obj
ninja: build stopped: subcommand failed.
FATAL ERROR: command exited with status 1: /usr/bin/cmake --build /home/my_new_zephyr_folder/ArduinoCore-zephyr/build

Is there a complier option that needs to be set versus adding it to llext_esports.c?

@mjs513 mjs513 changed the title Arduino Giga SdFat Bench example fails to load and run correctly GIGA: SdFat Bench example fails to load and run correctly Dec 30, 2024
@KurtE
Copy link

KurtE commented Dec 30, 2024

@mjs513 and @facchinm - I thought I would try it out as I hacked up a faster version of the sdfat-beta for the zephyr code,
using the subclass of the SPI class.

I ran into the same issue:
I did not find any references to the sbrk within this code base. Others yes...

I got around that one for now by adding to the sketch:

extern "C" {
char  *sbrk(int incr) { return (char *)-1; }
}

After that one it ran into: __aeabi_ul2f
fixed that one by adding:
FORCE_EXPORT_SYM(__aeabi_ul2f);
Now ran into: __cxa_pure_virtual);

**Edit**: I see we have some dummy versions of these on Teensy.
extern "C" void __cxa_pure_virtual(void) __attribute__ ((__noreturn__));
extern "C" void __cxa_deleted_virtual(void) __attribute__ ((__noreturn__));

void __cxa_pure_virtual(void) {
  // We might want to write some diagnostics to uart in this case
  //std::terminate();
  abort();
}

void __cxa_deleted_virtual(void) {
  // We might want to write some diagnostics to uart in this case
  //std::terminate();
  abort();
}

Now the SDFat-beta bench sketch loads...

Wonder where we shoul define these mission symble?

@mjs513
Copy link
Author

mjs513 commented Dec 30, 2024

@KurtE - @facchinm

For some reason that does not surprise me. I can't help the feeling that we are patching symptoms and not fixing the real cause of the problem

@mjs513
Copy link
Author

mjs513 commented Dec 30, 2024

Started digging more - zephyr has it defined already

https://github.com/zephyrproject-rtos/zephyr/blob/0f948fdb1c8376529a4d196dd664eb583028832f/lib/cpp/minimal/cpp_virtual.c#L18

Probably other ones are defined someplace else

@mjs513
Copy link
Author

mjs513 commented Jan 7, 2025

Decided to revisit this issue after updating the GIGA Overlay to include:

CONFIG_CPP=y
CONFIG_STD_CPP17=y

issue seems to have disappeared. Bench.ino sketch now compiles and runs - however something seems terribly wrong with the results

Use a freshly formatted SD for best performance.

Type any character to start
Type is exFAT
Card size: 63.86 GB (GB = 1E9 bytes)

Manufacturer ID: 0X3
OEM ID: SD
Product: SC64G
Revision: 8.0
Serial number: 0XCE8B50DD
Manufacturing date: 12/2019

FILE_SIZE_MB = 5
BUF_SIZE = 512 bytes
Starting write test, please wait.

write speed and latency
speed,max,min,avg
KB/Sec,usec,usec,usec
64.88,4286027345,7867,558
65.01,4286027320,7871,439458

Starting read test, please wait.

read speed and latency
speed,max,min,avg
KB/Sec,usec,usec,usec
65.04,4286027314,7864,439456
64.80,4286027348,7864,569

Compare this to https://forum.arduino.cc/t/sdfat-tests-on-giga-r1/1198629?_gl=1*1jrv1n0*_up*MQ..*_ga*NTg4NDU3NDkxLjE3MzYyOTA2ODY.*_ga_NEXN8H46L5*MTczNjI5MDY4Ni4xLjAuMTczNjI5MDY4Ni4wLjAuMTcwMTY4OTc4Mw.. which has

FILE_SIZE_MB = 5
BUF_SIZE = 512 bytes
Starting write test, please wait.

write speed and latency
speed,max,min,avg
KB/Sec,usec,usec,usec
387.99,16777,1310,1317
387.75,1632,1312,1318

Starting read test, please wait.

read speed and latency
speed,max,min,avg
KB/Sec,usec,usec,usec
392.53,1330,1289,1303
392.66,1328,1289,1302

Hopefully someone can confirm its not an SPI issu

@KurtE
Copy link

KurtE commented Jan 7, 2025

Are you using the released version of SDFat or my updated version?

It may not matter, but for some of the stuff I was playing with, I enabled Async SPI and needed to add:

CONFIG_SPI_ASYNC=y
CONFIG_SPI_STM32_INTERRUPT=y

If I remember correctly without using my version of SDFat, the code goes down the rabbit hole of only one byte read at a time.
Which is real slow.

@mjs513
Copy link
Author

mjs513 commented Jan 8, 2025

@KurtE - @facchinm
Decided to run the sketch again but at 24 mhz and put a LA on it and some weird results:
Image

Clock for SPI1 (thats what I am testing on is not anywhere near specified clock speed?

Going to check SPI and see if anything is different.

Update: Seeing same thing on SPI as well

@mjs513
Copy link
Author

mjs513 commented Jan 8, 2025

Interesting thing is that if I run @KurtE simple SPI test sketch:
Image
clock looks correct but not sure why I am not getting errors on miso and mosid

@mjs513
Copy link
Author

mjs513 commented Jan 8, 2025

Test3 Using custom SPI sketch shows the same thing
Image

Believe @KurtE test is using DMA?

Have to see if I pulled in the transfer16 changes

@KurtE
Copy link

KurtE commented Jan 9, 2025

I am pretty sure, I know what is going on .... Again.
Simple sketch

#include <SPI.h>
#define SD_CS 6
void setup() {
  // put your setup code here, to run once:
  pinMode(SD_CS, OUTPUT);
  digitalWrite(SD_CS, HIGH);
  Serial.begin(115200);
  while (!Serial && millis() < 5000) {}
  Serial.println("Before SPI Begin");
  SPI.begin();
  Serial.println("After");
  pinMode(LED_BUILTIN, OUTPUT);

}

uint8_t buffer[64];
void loop() {
  digitalWrite(SD_CS, LOW);
  digitalWrite(LED_BUILTIN, HIGH);
  SPI.beginTransaction(SPISettings(1000000, MSBFIRST, SPI_MODE0));
  memset(buffer, 's', sizeof(buffer));
  SPI.transfer(buffer, sizeof(buffer));
  digitalWrite(SD_CS, HIGH);
  SPI.endTransaction();
  SPI.beginTransaction(SPISettings(30000000, MSBFIRST, SPI_MODE0));
  digitalWrite(SD_CS, LOW);
  memset(buffer, 'F', sizeof(buffer));
  SPI.transfer(buffer, sizeof(buffer));
  SPI.endTransaction();
  digitalWrite(LED_BUILTIN, LOW);
  digitalWrite(SD_CS, HIGH);
  delay(100);
}

Image

The issue is, we call, things like:
` spi_transceive(pspi->SPIDevice(), pspi->getConfig(), &buf_set, nullptr);

And if that config pointer is the same as the previous call, it does not check if anything within that config structure has
changed, and so we are left with the first time called.

I thought the code in endTrasaction whereit calls: spi_release(spi_dev, &config);
was clearing it out... But maybe not. Will take a look... Again

`

@KurtE
Copy link

KurtE commented Jan 10, 2025

@facchinm @mjs513 - I have a version of SPI, that is working now, in that it will actually change speeds. The issue is that if you send the same spi_config object to the SPI transfer operations, it sees that the pointer is the same as the previous call, and does not
check/configure any differences...

Not sure the best way to do so: But my current version has the context object (ctx) store in addition to the config pointer,
it also has the config_frequency and the config_operation fields that it copied out of the config object and the
spi_context_configured checks those values, to see if it should do any updated configuration.

I also noticed that there are two copies of the spi_context.h file, one in the zephyr project and one in the ArduinoCore-zephyr project in the variant. I edited both, but suspect the zephyr one is copied in...

--- a/drivers/spi/spi_context.h
+++ b/drivers/spi/spi_context.h
@@ -31,6 +31,10 @@ struct spi_context {
 	const struct gpio_dt_spec *cs_gpios;
 	size_t num_cs_gpios;
 
+	/* how to test if the config changed? */
+	uint32_t config_frequency;
+	spi_operation_t config_operation;
+
 	struct k_sem lock;
 	struct k_sem sync;
 	int sync_status;
@@ -78,7 +82,7 @@ struct spi_context {
 static inline bool spi_context_configured(struct spi_context *ctx,
 					  const struct spi_config *config)
 {
-	return !!(ctx->config == config);
+	return !!((ctx->config == config) && (ctx->config_frequency == config->frequency) && (ctx->config_operation == config->operation));
 }
 
 static inline bool spi_context_is_slave(struct spi_context *ctx)
diff --git a/drivers/spi/spi_ll_stm32.c b/drivers/spi/spi_ll_stm32.c
index f509115592e..5c02b9191bb 100644
--- a/drivers/spi/spi_ll_stm32.c
+++ b/drivers/spi/spi_ll_stm32.c
@@ -582,6 +582,7 @@ static int spi_stm32_configure(const struct device *dev,
 	uint32_t clock;
 	int br;
 
+	//LOG_INF("spi_stm32_configure: %u %u", data->ctx.config_frequency, config->frequency);
 	if (spi_context_configured(&data->ctx, config)) {
 		/* Nothing to do */
 		return 0;
@@ -629,6 +630,7 @@ static int spi_stm32_configure(const struct device *dev,
 		}
 	}
 
+	//LOG_INF("Freq %u < %u < %u : br:%u", clock >> 1, config->frequency,clock >> ARRAY_SIZE(scaler), br);
 	if (br > ARRAY_SIZE(scaler)) {
 		LOG_ERR("Unsupported frequency %uHz, max %uHz, min %uHz",
 			    config->frequency,
@@ -701,6 +703,8 @@ static int spi_stm32_configure(const struct device *dev,
 
 	/* At this point, it's mandatory to set this on the context! */
 	data->ctx.config = config;
+	data->ctx.config_frequency = config->frequency;
+	data->ctx.config_operation = config->operation;
 
 	LOG_DBG("Installed config %p: freq %uHz (div = %u),"
 		    " mode %u/%u/%u, slave %u",

Note: I changed the test sketch to output the high speed twice, so I could check to see if it caught that it was same...

#include <SPI.h>
#define SD_CS 6
void setup() {
  // put your setup code here, to run once:
  pinMode(SD_CS, OUTPUT);
  digitalWrite(SD_CS, HIGH);
  Serial.begin(115200);
  while (!Serial && millis() < 5000) {}
  Serial.println("Before SPI Begin");
  SPI.begin();
  Serial.println("After");
  pinMode(LED_BUILTIN, OUTPUT);

}

uint8_t buffer[64];
void loop() {
  digitalWrite(SD_CS, LOW);
  digitalWrite(LED_BUILTIN, HIGH);
  SPI.beginTransaction(SPISettings(1000000, MSBFIRST, SPI_MODE0));
  memset(buffer, 's', sizeof(buffer));
  SPI.transfer(buffer, sizeof(buffer));
  digitalWrite(SD_CS, HIGH);
  SPI.endTransaction();
  SPI.beginTransaction(SPISettings(20000000, MSBFIRST, SPI_MODE0));
  digitalWrite(SD_CS, LOW);
  memset(buffer, 'F', sizeof(buffer));
  SPI.transfer(buffer, sizeof(buffer));
  SPI.endTransaction();
  digitalWrite(LED_BUILTIN, LOW);
  digitalWrite(SD_CS, HIGH);
  SPI.beginTransaction(SPISettings(20000000, MSBFIRST, SPI_MODE0));
  digitalWrite(SD_CS, LOW);
  memset(buffer, '2', sizeof(buffer));
  SPI.transfer(buffer, sizeof(buffer));
  SPI.endTransaction();
  digitalWrite(LED_BUILTIN, LOW);
  digitalWrite(SD_CS, HIGH);
  delay(100);
}

And LA shows obvious place where speed changed.
Image

I also verified that the picture viewer sketch was seeing the updated speed as well

@KurtE
Copy link

KurtE commented Jan 10, 2025

Follow on, in case we don't want to update the zephyr project to fix this. Another approach I was considering is to do it all within the SPI object. Sort of expand on the fix for 8 bit and 16 bit writes.

Have two config objects, and any time we change something, like speed or 8/16 bit output. We swap which of the two
config objects is used. Would not be hard. Can try that way if you would prefer.

@mjs513
Copy link
Author

mjs513 commented Jan 10, 2025

@KurtE @facchinm

Made the changes recommended and seems to work at least up to 24Mhz with Bench.ino. Anything larger than 24mhz seems to make the clock act funny. So looks like right now spi speed is capped at 24Mhz. NOTE: Did run at 12mhz and no issue clock acted like it should

At 12MHZ
Image

At 50Mhz
Image

@KurtE
Copy link

KurtE commented Jan 10, 2025

If I am reading the code correctly, I am assuming that the possible SPI speeds are:

120MHZ -  #define LL_SPI_BAUDRATEPRESCALER_DIV2              (0x00000000UL)
60MHZ -  #define LL_SPI_BAUDRATEPRESCALER_DIV4              (SPI_CFG1_MBR_0)
30MHZ -  #define LL_SPI_BAUDRATEPRESCALER_DIV8              (SPI_CFG1_MBR_1)
15MHZ -  #define LL_SPI_BAUDRATEPRESCALER_DIV16             (SPI_CFG1_MBR_1 | SPI_CFG1_MBR_0)
7.5MHZ -  #define LL_SPI_BAUDRATEPRESCALER_DIV32             (SPI_CFG1_MBR_2)
3.75MHZ -  #define LL_SPI_BAUDRATEPRESCALER_DIV64             (SPI_CFG1_MBR_2 | SPI_CFG1_MBR_0)
1.875MHZ -  #define LL_SPI_BAUDRATEPRESCALER_DIV128            (SPI_CFG1_MBR_2 | SPI_CFG1_MBR_1)
0.9375MHZ -  #define LL_SPI_BAUDRATEPRESCALER_DIV256            (SPI_CFG1_MBR_2 | SPI_CFG1_MBR_1 | SPI_CFG1_MBR_0)
```
Which changing the above to 60mhz appears to confirm that is a valid speed:

![Image](https://github.com/user-attachments/assets/a7bbdb3b-55d4-470a-a1c0-51d74377c7f7)

@mjs513
Copy link
Author

mjs513 commented Jan 10, 2025

Even stranger ran it at several clock (bench.ino)
60, 40, and 15: gave me sd card errors:

Type any character to start
begin() failed
Do not reformat the SD.
SdError: 0XC,0X0

other speeds:

30Mhz - 25mhz
FILE_SIZE_MB = 5
BUF_SIZE = 512 bytes
Starting write test, please wait.

write speed and latency
speed,max,min,avg
KB/Sec,usec,usec,usec
268.12,4286021356,1907,75
268.50,4286021349,1901,73

Starting read test, please wait.

read speed and latency
speed,max,min,avg
KB/Sec,usec,usec,usec
261.96,4286021402,1949,121
261.98,4286021402,1953,121

=======================================

12 mhz - 12.5mhz LA clock
write speed and latency
speed,max,min,avg
KB/Sec,usec,usec,usec
203.53,4286021959,2510,439598
204.87,4286021945,2496,664

Starting read test, please wait.

read speed and latency
speed,max,min,avg
KB/Sec,usec,usec,usec
201.65,4286021986,2475,706
201.87,4286021986,2475,439619

=============================
24mhz - 25mhz
FILE_SIZE_MB = 5
BUF_SIZE = 512 bytes
Starting write test, please wait.

write speed and latency
speed,max,min,avg
KB/Sec,usec,usec,usec
241.13,4286021570,2122,289
242.02,4286021562,2114,439198

Starting read test, please wait.

read speed and latency
speed,max,min,avg
KB/Sec,usec,usec,usec
239.18,4286021587,2113,307
239.18,4286021591,2113,307

===================================
36mhz 
FILE_SIZE_MB = 5
BUF_SIZE = 512 bytes
Starting write test, please wait.

write speed and latency
speed,max,min,avg
KB/Sec,usec,usec,usec
266.51,4286021368,1919,87
265.84,4286021369,1920,92

Starting read test, please wait.

read speed and latency
speed,max,min,avg
KB/Sec,usec,usec,usec
262.35,4286021398,1949,118
261.98,4286021401,1953,121

Looks like if you want better performance probably have to do your custom spi route

@mjs513
Copy link
Author

mjs513 commented Jan 11, 2025

@KurtE - @facchinm
Using Kurt's suggestion to USE_SPI_ARRAY_TRANSFER 1 got much better results:

Image

Manufacturer ID: 0X3
OEM ID: SD
Product: SC64G
Revision: 8.0
Serial number: 0XCE8B50DD
Manufacturing date: 12/2019

FILE_SIZE_MB = 5
BUF_SIZE = 512 bytes

Starting write test, please wait.
write speed and latency

speed,max,min,avg
KB/Sec,usec,usec,usec
1302.00,4286019834,380,439309
1312.94,1072,382,389

Starting read test, please wait.

read speed and latency
speed,max,min,avg
KB/Sec,usec,usec,usec
1344.00,4286019827,380,439297
1344.00,385,380,380
Done

Note: in bench.ino commented out Freestack calls, using Kurt's SPI setClock changes and the open pr incorporated

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

No branches or pull requests

2 participants