Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Arduino.h boolean type #5440

Closed
5 of 6 tasks
mcspr opened this issue Dec 5, 2018 · 9 comments
Closed
5 of 6 tasks

Arduino.h boolean type #5440

mcspr opened this issue Dec 5, 2018 · 9 comments

Comments

@mcspr
Copy link
Collaborator

mcspr commented Dec 5, 2018

Basic Infos

  • This issue complies with the issue POLICY doc.
  • I have read the documentation at readthedocs and the issue is not addressed there.
  • I have tested that the issue is present in current master branch (aka latest git).
  • I have searched the issue tracker for a similar issue.
  • If there is a stack dump, I have decoded it.
  • I have filled out all fields below.

Platform

Settings in IDE

  • Module: Generic ESP8266 Module

Problem Description

I had noticed that the boolean is typedefed to uint8_t, not bool as one would think:

typedef uint8_t boolean;

Arduino base repo had an issue about this problem: arduino/Arduino#2147

Gist of the original issue is that boolean & bool comparison through == may yield different than expected results. Just like this wiki article describes benefits of having native bool vs. implementing it through int: https://en.wikipedia.org/wiki/Boolean_data_type#C,_C++,_Objective-C,_AWK

The language guarantees that any two true values will compare equal (which was impossible to achieve before the introduction of the type).

AVR, SAMD etc. Cores have it typedef'd to bool since. After that, arduino/Arduino#4673 was raised about replacing boolean with bool all throughout the Cores.

So my questions are:

  • will PR simply replacing uint8_t -> bool in Arduino.h break something here?
  • should boolean used here (examples, core etc.) be replaced by bool?

MCVE Sketch

From the issue mentioned above, slightly modified for verbosity.
Conditions resolve the same ofc.

#include <Arduino.h>

#define CHECK(condition) Serial.print(#condition " is "); Serial.println((condition) ? "true" : "false");

void setup() {
    Serial.begin(115200);
    CHECK(true == (bool)57);
    CHECK(true == (boolean)57);
    CHECK(true == bool( true ));
    CHECK(bool( 1 | 2 | 4 | 8 ) == boolean( 1 | 2 | 4 | 8 ));
    CHECK(true == false + 1);
    CHECK(true + true == true);
    CHECK(bool( true + true ) == true);
    CHECK(boolean( true + true ) == true);
}
void loop() {}

Debug Messages

true == (bool)57 is true
true == (boolean)57 is false
true == bool( true ) is true
bool( 1 | 2 | 4 | 8 ) == boolean( 1 | 2 | 4 | 8 ) is false
true == false + 1 is true
true + true == true is false
bool( true + true ) == true is true
boolean( true + true ) == true is false
@devyte
Copy link
Collaborator

devyte commented Dec 5, 2018

I wholeheartedly agree with this. However, I've come across way too many libs out there that use boolean, and those would break.
I'll target this for release 3.0.0.

@devyte devyte added this to the 3.0.0 milestone Dec 5, 2018
@devyte devyte self-assigned this Dec 5, 2018
@TD-er
Copy link
Contributor

TD-er commented Dec 5, 2018

I think you may also break alignments in structs.
This will be a problem when those structs will be stored and read again.

@devyte
Copy link
Collaborator

devyte commented Dec 5, 2018

That is correct. Also, structs written to/read from EEPROM and rtcmem.

@earlephilhower
Copy link
Collaborator

We discussed this on gitter and also did some checks in GCC code and our own tests.

sizeof(boolean) == sizeof(bool) == 1

So alignment issues should not be a problem. There is still some concern about value interpretation, but I think that's actually the point of this bug, where bool will give a saner interpretation. Still some concern about what happens if you read back a pre-change boolean whose value was, say, "62" and not "1". Simple tests should verify this either works or not.

@TD-er
Copy link
Contributor

TD-er commented Jan 16, 2019

sizeof will always return the same indeed, but as soon as you're dealing with a struct with the packed pragma/attribute, then it really makes a difference whether it is an uint8_t or a bool.
Also it may be interesting to see what the sizeof is of:

  • bool[32] vs boolean[32]
  • struct with 32 members of type bool vs boolean.

@earlephilhower
Copy link
Collaborator

earlephilhower commented Jan 16, 2019

In GCC compiler source code when BOOL_TYPE_SIZE undefined, it's defined to CHAR_TYPE_SIZE. It's undefined in xtensa.h, so to GCC all packing/physical layout will be identical between char, uint8_t, and bool.

Even #pragma packed should behave the same because the layout rules will have the same input be it boolean or bool or char.

Some of the confusion may be that the linker will place consecutive uint8_ts at separate 4-byte alignment on Xtensa and not pack it (like you'd have on an 8-bit CPU like the AVR). So char a, b; takes 8 bytes of heap even though char a[2] only takes 4.

@earlephilhower
Copy link
Collaborator

typedef struct {
  boolean b;
  uint16_t u;
} __attribute__((__packed__)) s_bp;

typedef struct {
  bool b;
  uint16_t u;
} __attribute__((__packed__))  s_cp;

void setup() {
Serial.begin(115200);
Serial.printf("offsetof s_bp.u=%d\n", offsetof(s_bp, u));
Serial.printf("offsetof s_cp.u=%d\n", offsetof(s_cp, u));
}
void loop() { }

Generates:

offsetof s_bp.u=1
offsetof s_cp.u=1

@TD-er
Copy link
Contributor

TD-er commented Jan 17, 2019

I just did a test with these:

typedef struct {
  boolean b;
  boolean c;
  boolean d[16];
  uint16_t u;
} __attribute__((__packed__)) s_bp;

typedef struct {
  bool b;
  bool c;
  bool d[16];
  uint16_t u;
} __attribute__((__packed__))  s_cp;

With only bool/boolean b & c, both reported an offset of 2.
With the array d added both have an offset of 18.
So it seems both bool and boolean are indeed treated as 1 byte in size.

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 17, 2019

So there are no mis-alignment issue.
Only issues left would be that some boolean users assume a boolean could be different from 0 or 1.
I'd vote to push this change just after release 2.5.0.

@earlephilhower earlephilhower modified the milestones: 3.0.0, 2.6.0 Jan 22, 2019
earlephilhower pushed a commit to earlephilhower/Arduino that referenced this issue Jan 29, 2019
Match current Arduino definition to avoid issues with comparison
operations.

arduino/Arduino#2147
arduino/Arduino@20ac20f

Fixes esp8266#5440
devyte pushed a commit that referenced this issue Feb 7, 2019
Match current Arduino definition to avoid issues with comparison
operations.

arduino/Arduino#2147
arduino/Arduino@20ac20f

Fixes #5440
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants