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.

Sign up for GitHub

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

Jump to bottom

bitwise and operator used on sequential enum values #301

Open
denisbaylor opened this issue Dec 19, 2020 · 3 comments · May be fixed by #324
Open

bitwise and operator used on sequential enum values #301

denisbaylor opened this issue Dec 19, 2020 · 3 comments · May be fixed by #324
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project

Comments

@denisbaylor
Copy link

Throughout WiFi.cpp, the _status variable is tested using the bitwise and operator, but its possible values are not bitmasks. This looks wrong.

For example, at line 450 we have
if (!(_status & WL_CONNECTED)) {

The value of WL_CONNECTED is 3, so the expression (_status & WL_CONNECTED) is true if either of the lowest order 2 bits is set in _status. That makes line 450 behave the same as:

if (!(_status == WL_NO_SSID_AVAIL || _status == WL_SCAN_COMPLETED || _status == WL_CONNECTED || _status == WL_CONNECTION_LOST || _status == WL_DISCONNECTED || _status == WL_AP_LISTENING || _status == WL_AP_FAILED || _status == WL_PROVISIONING || _status == WL_PROVISIONING_FAILED)) {

And since line 446 looks like it intended to break out of the loop when _status was either WL_CONNECTED or WL_DISCONNECTED (but not quite because the same bug exists there), then line 450's if statement never succeeds because it doesn't distinguish between those two values. I believe the intended behavior of line 450 is instead

if (!(status_ == WL_CONNECTED)) {

or the simpler

if (status != WL_CONNECTED) {
@JAndrassy
Copy link

it is strange, but it doesn't create a bug since only one enum value is assigned to status_ at time

@denisbaylor
Copy link
Author

It doesn't matter that only one enum value is assigned to status_ at a time.

If I do:
status_ = WL_CONNECTED;

And then later do
if (status_ & WL_DISCONNECTED)

That condition will be true. This is because WL_CONNECTED is 3, WL_DISCONNECTED is 6,
and both of those values in binary have the 2 bit on.

@JAndrassy JAndrassy linked a pull request Jan 12, 2022 that will close this issue
Open
@JAndrassy
Copy link

you are right

@per1234 per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

WiFi._status condition expressions fix JAndrassy/WiFi101
3 participants
@per1234 @JAndrassy @denisbaylor