Typo in Arduino.h constant TIM_DIV265

[paccerdk]

I believe line 90 in Arduino/cores/esp8266/Arduino.h (Master) should be:
#define TIM_DIV256 3 //312.5Khz (1 tick = 3.2us - 26843542.4 us max)
and not:
#define TIM_DIV265 3 //312.5Khz (1 tick = 3.2us - 26843542.4 us max)
which it is, currently. (256 and not 265)

I did not submit a pull request or anything, since it might break stuff if corrected.

https://github.com/esp8266/Arduino/blob/master/cores/esp8266/Arduino.h#L90

[devyte]

@igrr I think I saw a discussion about this somewhere, but I can't find anything in this repo. Do you know anything about it? If memory serves, the discussion was like @paccerdk says: should the typo be fixed or kept for compatibility to not break people's code.
I think this should be fixed. Having code out there depend on a bug or typo is never a good idea (history lessons). At some point, code that currently depends on it may migrate to a newer release or git. At that point it won't compile, they will search for the reason, and they can correct any (wrong) dependencies then.

[igrr]

Thinking of something like

enum {
   TIM_DIV1 = 1,
   TIM_DIV16 = 2,
   TIM_DIV256 = 3,
   TIM_DIV265 __attribute__((deprecated)) = TIM_DIV256
};

and then remove TIM_DIV265 in one of the subsequent versions, giving e.g. library authors some time to update the code.

[devyte]

Relevant: PR #2695

[devyte]

@igrr I tried the enum deprecation that you suggested, and it didn't build. A bit of google says that gcc >= 6 is needed.
I tried a couple of variations, but they didn't work or didn't trigger a warning.
I created #4318 for now. If there's a way that works with our gcc, please let me know and I'll give it a shot. Otherwise, I suggest merging the PR now, and deprecating once we move to a newer gcc.
Also, I suggest removing the wrong 265 for milestone 3.0.0 (open new issue?).

[d-a-v]

with a const that would be possible (link) with our gcc.

[devyte]

I tried something similar to:
static const TIM_DIV265 = TIM_DIV256;
in Arduino.h, then this in a sketch:
const int tt = TIM_DIV265;

The warning didn't trigger, so I left it out. Not sure if I missed something or what.
In any case, I think a static const in Arduino.h would mean 4 bytes extra in every translation unit that includes it. Not sure it's worth it.

[d-a-v]

For the warning you need deprecated
extern "C" const int TIM_DIV265 __attribute__((deprecated, weak)) = TIM_DIV256;
(weak because we are in a .h)

The linker may remove the int if it not used. I did not check that.

[devyte]

@d-a-v I meant I tried this in Arduino.h
static const int TIM_DIV265 attribute((deprecated, weak)) = TIM_DIV256;

and:
const int tt = TIM_DIV265;

and the warning didn't trigger.

[d-a-v]

Weird, it does here (with the arduino IDE, except if File>Prefs>Warnings>None)

#undef TIM_DIV265
extern "C" const int TIM_DIV265 __attribute__((deprecated, weak)) = 3;
const int tt = TIM_DIV265;
const int uu = BUILTIN_LED;

test.ino:35:16: warning: 'TIM_DIV265' is deprecated (declared at test.ino:33) [-Wdeprecated-declarations]
 const int tt = TIM_DIV265;
                ^
test.ino:36:16: warning: 'BUILTIN_LED' is deprecated (declared at ...esp8266/variants/generic/common.h:78) [-Wdeprecated-declarations]
 const int uu = BUILTIN_LED;
                ^

[d-a-v]

... and it seems these two const int are not stored in flash. If I Serial.println() both of them, or only uu twice with the local tt/TIM_DIV265 definition commented, final binary size does not change.
A gcc underground specialist may confirm whether this is a gcc optimisation.

[d-a-v]

To be sure, I tried in Arduino.h.

Oddwierdly, it needs to be declared exactly as BUILTIN_LED with the #ifdef _cplusplus outside extern "C" { because inside and without extern "C" gcc complains with:

error: weak declaration of 'TIM_DIV2' must be public