-
Notifications
You must be signed in to change notification settings - Fork 21
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
Fixes for client non-persistence and blocking client.available() #259
base: dev
Are you sure you want to change the base?
Conversation
This fixes the broken rule of 3/5/0 in WiFiClient by removing the unneeded destructor, which caused the socket to stop prematurely on copy assignment. It also fixes the blocking behavior caused by `client.available()` by removing the unneeded `try_again` label and its corresponding `goto`. In its place, we stop the socket (already pending stop) instead. Closes Ameba-AIoT#257, Ameba-AIoT#258
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, hope this message finds you well. Congrats to your Pull Request! Thank you for the work. Your contributions have been outstanding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback.
Would you consider to use void WiFiClient::setBlockingMode()
and void WiFiClient::setNonBlockingMode()
for the update? Keep both situation. This class private value may help _is_blocked
Hi, @M-ichae-l Apologies for the delayed reply. I wasn't available for the last couple of days, and I wanted to confirm the following with my equipment before I replied to your comment: Upon testing with my AMB82-Mini board, I was able to confirm that the proposed changes do not affect the operation of |
@@ -95,14 +90,14 @@ int WiFiClient::available() | |||
return 0; | |||
} | |||
if (_sock >= 0) { | |||
try_again: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to suggest to use _is_blocked
by if condition, to keep both the update and original
@@ -66,11 +66,6 @@ WiFiClient::WiFiClient(uint8_t sock, tProtMode portMode, tBlockingMode blockMode | |||
_is_blocked = blockMode; | |||
} | |||
|
|||
WiFiClient::~WiFiClient() | |||
{ | |||
stop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason that remove this stop
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about the following update
if (_is_blocked) {
stop();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@redelacruz I think the suggested fix is ok. How about you?
revert: reinstates the try_again label and the relevant goto This reinstates the `try_again` label and the relevant `goto` statement as requested by @M-ichae-l in comment #r1713128977.
feat: stops socket if the client isn't in blocking mode When `WiFiClient::available()` is called, this stops the socket if `clientdrv.getLastErrno(_sock)` returns `EAGAIN` and the client isn't in blocking mode.
This pull request is stale because there is no activity for 14 days |
Description of Change
WiFiClient
by removing the unneeded destructor, which caused the socket to stop prematurely on copy assignment.client.available()
by removing the unneededtry_again
label and its correspondinggoto
. In its place, we stop the socket (already pending stop) instead.WiFiClient
no longer has a destructor. This is congruent with Arduino's design for this API, as you must now callclient.stop()
to close the connection (see [1], [2], [3]).Tests and Environments
Remark related links
Closes #257, closes #258