Skip to content

Fix freeze bug in FlashKeyValue.ino #32

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

Merged
merged 1 commit into from
Jun 1, 2023
Merged

Fix freeze bug in FlashKeyValue.ino #32

merged 1 commit into from
Jun 1, 2023

Conversation

alrvid
Copy link
Contributor

@alrvid alrvid commented Jun 1, 2023

The old version contained code like this: Serial.println("FlashIAP block device size: " + blockDevice.size());

This adds a large number (the block device size) to the address of the string literal before the + sign. It becomes an illegal address, which triggers a BusFault exception, and the sketch freezes after printing "FlashIAPBlockDevice + TDBStore Test."

The old version contained code like this: Serial.println("FlashIAP block device size: " +  blockDevice.size());

This adds a large number (the block device size) to the address of the string literal before the + sign. It becomes an illegal address, which triggers a BusFault exception, and the sketch freezes after printing "FlashIAPBlockDevice + TDBStore Test."
@CLAassistant
Copy link

CLAassistant commented Jun 1, 2023

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link

github-actions bot commented Jun 1, 2023

Memory usage change @ 7c3004b

Board flash % RAM for global variables %
arduino:mbed_edge:edge_control 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_portenta:envie_m7:target_core=cm4 N/A N/A N/A N/A
arduino:mbed_portenta:envie_m7 N/A N/A N/A N/A
Click for full report table
Board examples/Dual Core Processing/BlinkBothCores
flash
% examples/Dual Core Processing/BlinkBothCores
RAM for global variables
% examples/Edge Control Getting Started/EdgeControl
flash
% examples/Edge Control Getting Started/EdgeControl
RAM for global variables
% examples/Dual Core Processing/BlinkGreenLed_M4
flash
% examples/Dual Core Processing/BlinkGreenLed_M4
RAM for global variables
% examples/BLE Connectivity on Portenta H7/PortentaBLE
flash
% examples/BLE Connectivity on Portenta H7/PortentaBLE
RAM for global variables
% examples/Creating a Flash-Optimised Key-Value Store/FlashKeyValue
flash
% examples/Creating a Flash-Optimised Key-Value Store/FlashKeyValue
RAM for global variables
% examples/Creating GUIs with LVGL/lvglCounter
flash
% examples/Creating GUIs with LVGL/lvglCounter
RAM for global variables
% examples/Dual Core Processing/BlinkRedLed
flash
% examples/Dual Core Processing/BlinkRedLed
RAM for global variables
% examples/Dual Core Processing/BlinkRedLed_M7
flash
% examples/Dual Core Processing/BlinkRedLed_M7
RAM for global variables
% examples/Portenta H7 as a USB Host/LEDKeyboardController
flash
% examples/Portenta H7 as a USB Host/LEDKeyboardController
RAM for global variables
% examples/Portenta H7 as a WiFi Access Point/SimpleWebServer
flash
% examples/Portenta H7 as a WiFi Access Point/SimpleWebServer
RAM for global variables
% examples/Setting Up Portenta H7 For Arduino/Blink
flash
% examples/Setting Up Portenta H7 For Arduino/Blink
RAM for global variables
% examples/Vision Shield to SD Card bmp/visionShieldBitmap
flash
% examples/Vision Shield to SD Card bmp/visionShieldBitmap
RAM for global variables
%
arduino:mbed_edge:edge_control 0 0.0 0 0.0 0 0.0 0 0.0
arduino:mbed_portenta:envie_m7:target_core=cm4 N/A N/A N/A N/A N/A N/A N/A N/A
arduino:mbed_portenta:envie_m7 N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A
Click for full report CSV
Board,examples/Dual Core Processing/BlinkBothCores<br>flash,%,examples/Dual Core Processing/BlinkBothCores<br>RAM for global variables,%,examples/Edge Control Getting Started/EdgeControl<br>flash,%,examples/Edge Control Getting Started/EdgeControl<br>RAM for global variables,%,examples/Dual Core Processing/BlinkGreenLed_M4<br>flash,%,examples/Dual Core Processing/BlinkGreenLed_M4<br>RAM for global variables,%,examples/BLE Connectivity on Portenta H7/PortentaBLE<br>flash,%,examples/BLE Connectivity on Portenta H7/PortentaBLE<br>RAM for global variables,%,examples/Creating a Flash-Optimised Key-Value Store/FlashKeyValue<br>flash,%,examples/Creating a Flash-Optimised Key-Value Store/FlashKeyValue<br>RAM for global variables,%,examples/Creating GUIs with LVGL/lvglCounter<br>flash,%,examples/Creating GUIs with LVGL/lvglCounter<br>RAM for global variables,%,examples/Dual Core Processing/BlinkRedLed<br>flash,%,examples/Dual Core Processing/BlinkRedLed<br>RAM for global variables,%,examples/Dual Core Processing/BlinkRedLed_M7<br>flash,%,examples/Dual Core Processing/BlinkRedLed_M7<br>RAM for global variables,%,examples/Portenta H7 as a USB Host/LEDKeyboardController<br>flash,%,examples/Portenta H7 as a USB Host/LEDKeyboardController<br>RAM for global variables,%,examples/Portenta H7 as a WiFi Access Point/SimpleWebServer<br>flash,%,examples/Portenta H7 as a WiFi Access Point/SimpleWebServer<br>RAM for global variables,%,examples/Setting Up Portenta H7 For Arduino/Blink<br>flash,%,examples/Setting Up Portenta H7 For Arduino/Blink<br>RAM for global variables,%,examples/Vision Shield to SD Card bmp/visionShieldBitmap<br>flash,%,examples/Vision Shield to SD Card bmp/visionShieldBitmap<br>RAM for global variables,%
arduino:mbed_edge:edge_control,0,0.0,0,0.0,0,0.0,0,0.0
arduino:mbed_portenta:envie_m7:target_core=cm4,N/A,N/A,N/A,N/A,,,,,N/A,N/A,N/A,N/A
arduino:mbed_portenta:envie_m7,N/A,N/A,N/A,N/A,,,,,,,,,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A

@per1234 per1234 added type: imperfection Perceived defect in any part of project topic: documentation Related to documentation for the project labels Jun 1, 2023
@sebromero sebromero self-requested a review June 1, 2023 14:48
Copy link
Collaborator

@sebromero sebromero left a comment

Choose a reason for hiding this comment

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

LGTM

@sebromero sebromero merged commit 90dd5f3 into arduino-libraries:main Jun 1, 2023
@aliphys
Copy link

aliphys commented Jun 2, 2023

Explanation of ChatGPT regarding the issue @alrvid 's solved:


Based on the provided code snippet, it appears that there is a bug in the line:

Serial.println("FlashIAP block device size: " +  blockDevice.size());

The bug lies in the concatenation operation between the string literal ("FlashIAP block device size: ") and the result of blockDevice.size(). In C++ and Arduino, the concatenation of strings using the + operator requires both operands to be of type String. However, blockDevice.size() likely returns a numerical value, such as an integer or a long.

To fix this issue, you need to convert the numerical value to a string before concatenating it with the string literal. One way to achieve this is by using the String() function to convert the numerical value to a String object. Here's the corrected code:

Serial.println("FlashIAP block device size: " + String(blockDevice.size()));

By wrapping blockDevice.size() with String(), you ensure that it is converted to a String object before concatenating it with the string literal.


In this PR, blockDevice.size() was moved to a separate line, which also solves this issue. :)

@sebromero
Copy link
Collaborator

@aliphys Likely ChatGPT is not correct here. The problem is with the string literal which is not of type String but char *. The latter one has no operator that supports concatenation with other values as strings. Instead, since it's a pointer it just increments the address. Another solution that should work would be this:

Serial.println(String("FlashIAP block device size: ") +  blockDevice.size());

This should work because the String class has an operator overload for + with numeric values. See:
https://github.com/arduino/ArduinoCore-API/blob/master/api/String.h#L130

@alrvid
Copy link
Contributor Author

alrvid commented Jun 2, 2023

The answer from ChatGPT is incorrect for two reasons:

  1. Both operands aren't required to be String - only one is. For some reason it ignores that it made this erroneous statement when suggesting the fix further down.

  2. The conversion String(blockDevice.size()) doesn't work, because blockDevice.size() is of type bd_size_t, which is typedef from uint64_t, and the String class doesn't have a constructor for it. The call is ambiguous - the compiler wouldn't be able to determine which overloaded constructor to use.

Both:

Serial.println("FlashIAP block device size: " + String((uint32_t) blockDevice.size()));

and:

Serial.println(String("FlashIAP block device size: ") + (uint32_t) blockDevice.size());

are possible, but slightly suboptimal workarounds compared to the separate print() plus println() solution, because they force the uint64_t into a uint32_t.

In the first case, the compiler performs an implicit conversion of the string literal to String because there's a String to the right and there's a conversion operator defined from const char * to String. The compiler then uses the + operator overload of the implicitly converted result together with the second string that results from passing a uint32_t to the String constructor.

In the second case, the compiler simply uses the + operator overload of the String to the left together with the uint32_t.

The reason why Serial.println(blockDevice.size()) in the pull request works is that there's an overloaded version of println() that takes an unsigned long long. Thus, there is no loss of precision compared to the other two solutions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: documentation Related to documentation for the project type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants