Skip to content

Conversation

dok-net
Copy link
Contributor

@dok-net dok-net commented May 31, 2021

Related to #8089

Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

Not a fan of the way Arduino did it, but this breaks Arduino compatibility.

https://github.com/arduino/ArduinoCore-API/blob/173e8eadced2ad32eeb93bcbd5c49f8d6a055ea6/api/Common.h#L151-L153

uint16_t makeWord(uint16_t w); uint16_t makeWord(byte h, byte l); 

Removing byte elsewhere seems fine, but the 2-param makeword seems like it should be moved from unsigned char to byte in WMath.cpp.

@dok-net
Copy link
Contributor Author

dok-net commented May 31, 2021

In the light of even, say:

#define lowByte(w) ((uint8_t) ((w) & 0xff)) #define highByte(w) ((uint8_t) ((w) >>; 8)) 

using uint8_t, I find it all rather inconsistent to begin with. Also, while C++ (at least when I read the specs a long time ago, LOL) was based on type-name, not type-structure, if you like to convince me please explain the difference that it makes, when all the while

typedef uint8_t byte; 

lets the compiler cast and then link just fine. My reckoning in this is to get rid of "byte" as much as possible - rationale (byte is used as object ident) was given above.
As a compromise, we could leave Arduino.h as it is ...

By the way, I messed up, doing the ESP32 in kind of at the same time, and didn't fix the return type inconsistency here in ESP8266 Core. I have done that now (fixup and rebase).

There are other indications that this discussion is moot, like (AVR Arduino.h to ESP8266's):

-typedef unsigned int word; +typedef uint16_t word; 

and the AVR Arduino.h and ESP8266 Arduino.h are not really very much alike anymore overall. So keep things straight and simple, let's adapt in Arduino.h to uint8_t, too, please.

@dok-net dok-net requested a review from earlephilhower May 31, 2021 21:12
@dok-net dok-net force-pushed the fix_8089 branch 2 times, most recently from 1f3a19b to 8b48530 Compare June 3, 2021 16:43
@dok-net
Copy link
Contributor Author

dok-net commented Jun 3, 2021

Regarding Arduino compatibility: https://github.com/arduino/ArduinoCore-API
Loads of unrewarding differences to merge, figure out, repair, refactor.

@dok-net dok-net force-pushed the fix_8089 branch 3 times, most recently from a5b8112 to c00faa0 Compare June 8, 2021 01:46
Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

LGTM

@earlephilhower earlephilhower added this to the 3.1 milestone Jun 17, 2021
@d-a-v d-a-v modified the milestones: 3.1, 3.0.2 Jul 19, 2021
@d-a-v d-a-v merged commit bc30251 into esp8266:master Jul 26, 2021
dperish added a commit to dperish/WiFiManager that referenced this pull request Sep 21, 2021
Change byte to uint8_t to cope with breaking change in 3.2.0 of platform: esp8266/Arduino#8090
@dok-net dok-net deleted the fix_8089 branch October 9, 2021 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants