Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Pull in SDFat patch from ESP8266 #1150

Closed
earlephilhower opened this issue Jan 31, 2023 · 5 comments
Closed

Pull in SDFat patch from ESP8266 #1150

earlephilhower opened this issue Jan 31, 2023 · 5 comments

Comments

@earlephilhower
Copy link
Owner

esp8266/Arduino#8833

earlephilhower added a commit that referenced this issue Feb 8, 2023
Fixes #1150

Synchronize with ESP8266 implementation of SD.h
@mcspr
Copy link

mcspr commented Feb 8, 2023

@earlephilhower
Copy link
Owner Author

#1171 pulls in the logic change without the template/enum stuff which should take care of it. Thanks for reminding me!

@mcspr
Copy link

mcspr commented Feb 12, 2023

I mean that narrowing happens, access append mode gets lost after int -> u8
(that totally not template or enum scary c++ code from original forces build time checks)

@earlephilhower
Copy link
Owner Author

Sorry, but I'm not seeing it. If you're saying O_APPEND doesn't fit in u8, then the function prototype needs to change, no? But in the ESP826 PR I see it's still u8 param in the constant->string interpterer.

In Newlib 4.0, for O_APPEND I see it fits in u8:

system/arm-none-eabi/arm-none-eabi/include/sys/_default_fcntl.h:#define	O_APPEND	_FAPPEND
...
system/arm-none-eabi/arm-none-eabi/include/sys/_default_fcntl.h:#define	_FAPPEND	0x0008	/* append (writes guaranteed at the end) */

@mcspr
Copy link

mcspr commented Feb 13, 2023

Oops, wrong header, sorry.

Pulling toolchain from here, original issue where FILE_WRITE uses O_RDONLY | O_WRONLY (0x0 | 0x1) still happens, O_RDWR is 0x2

(re-pulling test lines)

/home/runner/.arduino15/packages/rp2040/hardware/rp2040/2.7.2/libraries/SD/src/SD.cpp:23:63: error: static assertion failed
   23 | static_assert(__builtin_strcmp(SDClassFileMode(FILE_WRITE), "a+") == 0, "");
      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~

Initial patch stumbled on O_CREAT (aka _FCREAT, 0x0200) and O_AT_END (aka _FNONBLOCK, 0x4000) being forced into u8, which is what SdFat uses in FILE_WRITE definition
https://github.com/earlephilhower/arduino-pico/actions/runs/4129029030/jobs/7134181046#step:5:274

inline constexpr uint8_t SDClassFileWrite { FILE_WRITE | O_APPEND };

Keeping preprocessor stuff might still cause random warnings, whenever function argument gets converted.

In file included from /home/runner/.arduino15/packages/rp2040/hardware/rp2040/2.7.2/libraries/SD/src/SD.cpp:1:
/home/runner/.arduino15/packages/rp2040/hardware/rp2040/2.7.2/libraries/SD/src/SD.h:29:48: warning: unsigned conversion from 'int' to 'uint8_t' {aka 'unsigned char'} changes value from '521' to '9' [-Woverflow]
   29 | #define FILE_WRITE (O_READ | O_WRITE | O_CREAT | O_APPEND)

C++ const just gives an option to be more careful about used type, instead of context-dependant text injection

ESP8266 PR missed the oflag_t stuff, would need to push that one to have consistent type from SdFat vs. our guess of what it is originally

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants