You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The F() flash macro used for storing strings in flash memory no longer appears to be working as of the 1.8.5 version. The following error is reported during compile for each use.
error: cannot convert 'const char*' to 'const __FlashStringHelper*' in return
#define F(str) (str)
@facchinm even though it would fix this exact issue, it's not a very elegant solution. Will this mess up other libraries that use __FlashStringHelper in some way?
I think it will, since libraries might overload a method for both char* and __FlashStringHelper*. It might be better to make this a separate type and heave F() cast to __FlashStringHelper* as well.
How does SAM and SAMD solve this? I suspect like above?
@facchinm, could you please explain a little further why the F() macro is useless for the megaAVR architecture? I did not understand what you meant by "only increases flash occupation". Both the Uno R3 and the WiFi R2 have flash memory; are they used differently?
Classic AVR architecture is referred as modified Harward, because data and memory are in different address spaces (so address 0 could refer to ram or flash, and there are different functions to access them).
New AVR (rebranded megaAVR) architecture is instead more conventional as flash and ram share the same address space (like ARM) so there's no need for special treatment if a variable is only in flash (adding const modifier is enough).
Anyway, __FlashStringHelper is clearly not intended to be used as-is but as an helper to print/compare strings in Flash with strings in RAM on a very peculiar architecture via the F() macro. Clearly it's too late to hide it but I have no better idea than defining it as const char or char.
Re option 1, the performance overhead might actually be even smaller than I thought. I was thinking lpm is three cycles and ld only 2, but that's only for loads for RAM. Apparently ld from the mapped flash is also three cycles (maybe even more, documentation is unclear). Using the mapped flash might still be faster since ld has more addressing modes and lds can be used when the address is known at compiletime, though.
Cycle time for data memory access assumes internal RAM access, and are not valid for access to NVM. A
minimum of one extra cycle must be added when accessing NVM. The additional time varies dependent on
the NVM module implementation. See the NVMCTRL section in the specific devices data sheet for more
information.
So that makes at least 3 cycles for ld from flash. However, the NVMCTRL section of the Atmega4808/9 datasheet does not seem to say anything about the number of cycles to read from mapped flash...
Giving this a bit more thought, and seeing the PRs that try to implement both approaches, I suspect that maybe option 1. is the better one after all. There is some overhead, but it should really work with all code out there (AFAICS) and does not complicate the include path even more, which is worth a few cycles IMHO.
I've been going through all those issues (#53, #62), PRs (#54, #82, #85, #87) and commits (a0f6beb) again and frankly, my head hurts 🤕. All in all it seems to me that by @facchinm 's commit a0f6beb we've implemented (restored actually) option 1 as described by @matthijskooijmanhere. @matthijskooijman can you confirm that I copy you correctly?
Also @giulcioffi's PRs are outdated a bit by #62 (comment) (this muddled the water when investigating this issue) - but no fault of her own, rather result of the long-standing PR we failed to integrate.
All in all it seems to me that by @facchinm 's commit a0f6beb we've implemented (restored actually) option 1 as described by @matthijskooijman here
Yes, I agree. Now, -API includes the main avr-libc pgmspace.h for PROGMEM and PSTR, and defines __FlashStringHelper and F() as on all platforms, and megaavr doesn't seem to change these, so I think now progmem stuff just works as it does on AVR, which I think is the indeed the safest approach. If we want to go for option 1, I think we're done and the issues and PRs can be closed.
There were some alternative PRs to compare the impact of this approach (but of those, I think only #85, implementing option 2, is relevant, since #87 has major compatibility problems, and #82 essentially just does what current master does with a bunch of duplicated code). So if we still want to consider option 2, we might want to look at the result of #85, but to really compare, that should be rebased on current master so it really compares option 1 with option 2 (it now compares the older, broken no-op F() version with option 2). Also, I think #85 (or rather, the https://github.com/giulcioffi/ArduinoCore-API/commits/FmacroIssue branch) needs some more work to really be ready for merging, but it looks ok for testing now.
Thank you very much for your feedback @matthijskooijman. Let's go with option #1 for now and schedule another look for this issue at a later point in time 🙏 .
Activity
facchinm commentedon Dec 3, 2019
F() is useless in megaAVR architecture (it only increases flash occupation). Which code are you trying to compile?
matthijskooijman commentedon Dec 3, 2019
But for compatibility with existing sketches, all other cores have a working
F()
macro as well, even when they do not actually need.facchinm commentedon Dec 3, 2019
Also this core has it, this is why I'm asking for the code 🙂
woolseyj commentedon Dec 3, 2019
The code is the Adafruit_IO_Arduino library.
facchinm commentedon Dec 3, 2019
I just compiled one random example from that library and I didn't get any error. Could you provide more info?
woolseyj commentedon Dec 3, 2019
Here is an example sketch that uses the library.
facchinm commentedon Dec 3, 2019
Got it. This patch makes the project compile again
But I'm not 100% sure it is the right approach.
@MCUdude do you think it could work as is?
woolseyj commentedon Dec 3, 2019
Confirmed.
MCUdude commentedon Dec 3, 2019
@facchinm even though it would fix this exact issue, it's not a very elegant solution. Will this mess up other libraries that use
__FlashStringHelper
in some way?matthijskooijman commentedon Dec 3, 2019
I think it will, since libraries might overload a method for both
char*
and__FlashStringHelper*
. It might be better to make this a separate type and heaveF()
cast to__FlashStringHelper*
as well.How does SAM and SAMD solve this? I suspect like above?
woolseyj commentedon Dec 3, 2019
@facchinm, could you please explain a little further why the F() macro is useless for the megaAVR architecture? I did not understand what you meant by "only increases flash occupation". Both the Uno R3 and the WiFi R2 have flash memory; are they used differently?
facchinm commentedon Dec 3, 2019
Classic AVR architecture is referred as modified Harward, because data and memory are in different address spaces (so address 0 could refer to ram or flash, and there are different functions to access them).
New AVR (rebranded megaAVR) architecture is instead more conventional as flash and ram share the same address space (like ARM) so there's no need for special treatment if a variable is only in flash (adding
const
modifier is enough).woolseyj commentedon Dec 3, 2019
Great, thanks!
facchinm commentedon Dec 3, 2019
Anyway,
__FlashStringHelper
is clearly not intended to be used as-is but as an helper to print/compare strings in Flash with strings in RAM on a very peculiar architecture via theF()
macro. Clearly it's too late to hide it but I have no better idea than defining it asconst char
orchar
.22 remaining items
matthijskooijman commentedon Sep 8, 2020
Cool!
Re option 1, the performance overhead might actually be even smaller than I thought. I was thinking
lpm
is three cycles andld
only 2, but that's only for loads for RAM. Apparentlyld
from the mapped flash is also three cycles (maybe even more, documentation is unclear). Using the mapped flash might still be faster sinceld
has more addressing modes andlds
can be used when the address is known at compiletime, though.As for the cycle time, the AVR Instruction Set Manual says:
So that makes at least 3 cycles for
ld
from flash. However, theNVMCTRL
section of the Atmega4808/9 datasheet does not seem to say anything about the number of cycles to read from mapped flash...matthijskooijman commentedon Nov 4, 2020
Giving this a bit more thought, and seeing the PRs that try to implement both approaches, I suspect that maybe option 1. is the better one after all. There is some overhead, but it should really work with all code out there (AFAICS) and does not complicate the include path even more, which is worth a few cycles IMHO.
aentinger commentedon Dec 11, 2020
I've been going through all those issues (#53, #62), PRs (#54, #82, #85, #87) and commits (a0f6beb) again and frankly, my head hurts 🤕. All in all it seems to me that by @facchinm 's commit a0f6beb we've implemented (restored actually) option 1 as described by @matthijskooijman here. @matthijskooijman can you confirm that I copy you correctly?
Also @giulcioffi's PRs are outdated a bit by #62 (comment) (this muddled the water when investigating this issue) - but no fault of her own, rather result of the long-standing PR we failed to integrate.
matthijskooijman commentedon Dec 11, 2020
Yes, I agree. Now, -API includes the main avr-libc
pgmspace.h
forPROGMEM
andPSTR
, and defines__FlashStringHelper
andF()
as on all platforms, and megaavr doesn't seem to change these, so I think now progmem stuff just works as it does on AVR, which I think is the indeed the safest approach. If we want to go for option 1, I think we're done and the issues and PRs can be closed.There were some alternative PRs to compare the impact of this approach (but of those, I think only #85, implementing option 2, is relevant, since #87 has major compatibility problems, and #82 essentially just does what current master does with a bunch of duplicated code). So if we still want to consider option 2, we might want to look at the result of #85, but to really compare, that should be rebased on current master so it really compares option 1 with option 2 (it now compares the older, broken no-op
F()
version with option 2). Also, I think #85 (or rather, the https://github.com/giulcioffi/ArduinoCore-API/commits/FmacroIssue branch) needs some more work to really be ready for merging, but it looks ok for testing now.aentinger commentedon Dec 14, 2020
Thank you very much for your feedback @matthijskooijman. Let's go with option #1 for now and schedule another look for this issue at a later point in time 🙏 .
woolseyj commentedon Dec 26, 2020
When is the next version expected to be released that will include this fix?
aentinger commentedon Jan 8, 2021
@facchinm What do you reckon?
facchinm commentedon Jan 8, 2021
@aentinger version 1.8.7 was released with the fix just before the end of the year 🙂
aentinger commentedon Jan 8, 2021
Totally true, I was deceived because it was released via cli
git tag
therefore not showing up in the release section of the repo main page 😊