Closed giulcioffi closed 3 years ago
matthijskooijman
commented
4 years ago I think this PR does not solve the problems illustrated at https://github.com/arduino/ArduinoCore-megaavr/issues/62#issuecomment-617079408 and subsequent comments. I haven't got time to re-read those comments and comment on a proper approach, but I think this PR is unfortunately not enough to make this work...
github-actions[bot]
commented
4 years ago Memory usage change @ 737660e1e889c2ea9e3a3e212e441fdc98fcd84d
| Board | flash | % | RAM for global variables | % |
|---|---|---|---|---|
| arduino:megaavr:nona4809 | :small_red_triangle: 0 - +86 | 0.0 - +0.17 | 0 - 0 | 0.0 - 0.0 |
| arduino:megaavr:uno2018:mode=off | :small_red_triangle: 0 - +86 | 0.0 - +0.18 | 0 - 0 | 0.0 - 0.0 |
| arduino:megaavr:uno2018:mode=on | :small_red_triangle: 0 - +86 | 0.0 - +0.18 | 0 - 0 | 0.0 - 0.0 |
github-actions[bot]
commented
4 years ago Memory usage change @ 666a6f627c4ab21d6bf6db6e7c6ed2e0bf27fab7
| Board | flash | % | RAM for global variables | % |
|---|---|---|---|---|
| arduino:megaavr:nona4809 | :small_red_triangle: 0 - +97 | 0.0 - +0.2 | 0 - 0 | 0.0 - 0.0 |
| arduino:megaavr:uno2018:mode=off | :small_red_triangle: 0 - +97 | 0.0 - +0.2 | 0 - 0 | 0.0 - 0.0 |
| arduino:megaavr:uno2018:mode=on | :small_red_triangle: 0 - +97 | 0.0 - +0.2 | 0 - 0 | 0.0 - 0.0 |
doctorames
commented
3 years ago This fixed my build issue. It may not be the best fix, as @matthijskooijman suggests, but at least I'm unblocked now and can continue with my project. Thanks.
aentinger
commented
3 years ago Question @giulcioffi : Should pgmspace.h not already be included via ArduinoCore-API?
giulcioffi
commented
3 years ago @aentinger ArduinoCore-API provides an implementation of pgmspace.h which is different from the one used by typical AVR architectures. As suggested here in option 1, implementing PROGMEM as on regular AVR should fix the issue #62 .
To do this, I decided to keep ArduinoCore-API as is and, instead, add the implementation of pgmspace.h from avr-libc in the tools folder and modify the include path in Arduino.h.
facchinm
commented
3 years ago I'm still unsure there's a perfect way to tackle this.
https://github.com/arduino/ArduinoCore-megaavr/commit/d316aa62ae4f515095319cd9e8b0ac781daf5fb8 was introduced to save ram and flash taking advantage of the new internal architecture. Restoring F macro by reverting that patch should at least keep the things tidy and shouldn't have any size disadvantage compared with https://github.com/arduino/ArduinoCore-megaavr/pull/82/commits/666a6f627c4ab21d6bf6db6e7c6ed2e0bf27fab7 .
Since the failure is happening in a very particular case (functions returning pointers to FlashStringHelper) I'd concentrate on that wrapper class more than on F macro)
matthijskooijman
commented
3 years ago I think that the goal of this PR, to make PROGMEM etc. work as defined by avr-libc unmodified, and make F() work as on regular Arduino AVR, is probably the safest and best approach.
However, I don't like the approach taken by this PR, namely to duplicate a file that's also shipped with avr-libc. I also wonder if it is even needed, since the api/deprecated-avr-comp is, AFAICS, not even put on the include path, as that directory is only meant for non-AVR cores that do not have the avr-libc-specific stuff.
So I would that simply removing the redefinition of F() from this core without the other changes, would end up using the avr-libc-supplied pgmspace.h and the API-supplied F() macro, which would work AFAICS (haven't tested, though).
Since the failure is happening in a very particular case (functions returning pointers to FlashStringHelper) I'd concentrate on that wrapper class more than on F macro)
@facchinm, what failure are you referring to exactly? I think the problem here is simply that the API exposed by the combination of PROGMEM, PSTR(), F(), __FlashStringHelper* and pgm_load_*() simply is not consistent and/or not compatible with other cores. Maybe this inconsistency only surfaces in particular cases, but I think those cases are real-world usecases, so cannot be disregarded. But maybe you propose something different?
aentinger
commented
3 years ago Closing, for details see https://github.com/arduino/ArduinoCore-megaavr/issues/62#issuecomment-743159584.
This PR is intended to solve isse #62. The F() macro has been implemented as in typical AVR architecture: in
String.hclass __FlashStringHelper;#define F(string_literal) (reinterpret_cast<const __FlashStringHelper *>(PSTR(string_literal)))pgmspace.his not included anymore from ArduinoCore-API/api/deprecated-avr-comp, but is the one provided by avr-libc. It has been copied from there and inserted into a new foldertools. This has been done to avoid modifications inside ArduinoCore-API.