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

wifi non blocking begin() #2063

Merged
merged 12 commits into from
Apr 19, 2024

Conversation

keever50
Copy link
Contributor

A very small addition where the WiFiClass begin() functions have been copied without the blocking part.
These new functions are begin_noblock.

The normal begin function blocks the microcontroller until wifi is connected. This can freeze code or trigger a watchdog.
Now using begin_noblock, the user can check for .connected themselfs.

I hope this helps!
Let me know if there are any feedback or problems

@keever50
Copy link
Contributor Author

Ill fix the checks!

@keever50
Copy link
Contributor Author

Passed the checks and changed the function name to beginNoBlock() like arduino style.

@earlephilhower
Copy link
Owner

I am OK with this addition, but can we do it in a way w/o lots of cut-n-paste? Maybe have the ::begin and ::beginNoBlock just call a private ::beginInternal function which takes, as a parameter, whether or not it should block? That way when changes come in there's only 1 spot to do the edits (i.e. we have done mods to WiFi for the ESP32-based wifi chips already).

@keever50
Copy link
Contributor Author

I agree! Will do that tomorrow. Good to know you are ok with this addition

@keever50
Copy link
Contributor Author

Added the _beginInternal to handle the normal begin. I had to modify the return values of this private in between step. Normally it returns WL_IDLE_STATUS on error, which is zero. It is the only possible error. Now it returns 1 on error, 0 on success.
But the public begin should behave as before for compatibility.

@keever50
Copy link
Contributor Author

I didn't actually say it here, but it's so far I know ready. Anything left to change?

@earlephilhower earlephilhower self-requested a review March 21, 2024 19:39
@earlephilhower
Copy link
Owner

Let me run it locally first, but give me a day or two, please.

@JAndrassy
Copy link
Contributor

sorry, why not just use 0 timeout?

@keever50
Copy link
Contributor Author

sorry, why not just use 0 timeout?

From my view, timeout==bad, so having a timeout out of the equation makes sense to me personally.

What if the timeout gets implemented somewhere else in the library where it is wanted? Maybe 0 timeout will conflict? A noblock function also leaves this question out of the equation.

I guess it depends on how one looks at it.

Let me know what you think

(Also, I didn't want to sound pushy on previous message. No worries, was making sure things are alright)

Copy link
Owner

@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.

Minor tweaks, but OTW LGTM.

libraries/WiFi/src/WiFiClass.cpp Outdated Show resolved Hide resolved
@earlephilhower earlephilhower self-requested a review April 19, 2024 16:37
It's generally frowned upon to have code that goes
`if (true) return true; else return false;`

Instead, simply return the `bool` result of the comparison
Copy link
Owner

@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.

Actually, now that I'm back home and checking this out, it seems this is changing all the return values from the wl_definitions into bool which is not appropriate and will break things. I'll see if I can undo the changes in the GH editor...

@keever50
Copy link
Contributor Author

I replied before and exactly on the same time when you were posting your message. Yep, I got a little confused with the error logic as well. Seems like the SDK kinda tends to prefer my previous way of handling errors. Return int as error, because there are more errors than successes. But Arduino really likes bools... I am not entirely sure what is the right method at the moment.

Copy link
Owner

@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.

OK, on further re-read I see it (the public methods) does still return wl_definition values. Mybad! The internal version can return whatever we want, it's only the interface to user code that needs to be preserved.

@earlephilhower earlephilhower merged commit 11dfb2c into earlephilhower:master Apr 19, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants