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

ESP8266 - fix send buffer exhaustion handling #9309

Merged
merged 1 commit into from
Jan 10, 2019
Merged

ESP8266 - fix send buffer exhaustion handling #9309

merged 1 commit into from
Jan 10, 2019

Conversation

VeijoPesonen
Copy link
Contributor

Description

Driver must return NSAPI_ERROR_WOULD_BLOCK if a server won't accept
data from the modem and the modem's buffers are full. In case that
socket is closed driver returns NSAPI_ERROR_CONNECTION_LOST.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@ARMmbed/mbed-os-ipcore

Copy link
Contributor

@michalpasztamobica michalpasztamobica left a comment

Choose a reason for hiding this comment

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

Just one optional improvement noticed.

ret = NSAPI_ERROR_OK;
}

END:
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using the goto statement, the ifs above could be transformed into an if - else if - else if structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Duly noted, but I'll keep the original for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of like the goto END type of handling where we have multiple cases which all should lead to shared error handling.
Its like try { ... } catch { ... } finally { ... }

Copy link
Contributor

Choose a reason for hiding this comment

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

In a previous life, I worked with do { ... } while (false). I've always had mixed feelings about it.

@ciarmcom ciarmcom requested review from a team January 9, 2019 14:00
@ciarmcom
Copy link
Member

ciarmcom commented Jan 9, 2019

@VeijoPesonen, thank you for your changes.
@ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

tr_debug("ESP8266::send(): connection disrupted");
}

if (!_sock_i[id].open) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Must actually be if (!_sock_i[id].open && ret != NSAPI_ERROR_OK) {


if (_error) {
ret = NSAPI_ERROR_CONNECTION_LOST;
tr_debug("ESP8266::send(): connection disrupted");
Copy link
Contributor

Choose a reason for hiding this comment

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

is this actually true?

Should we test that socket is actually closed? We should know that, because OOBs are already handled.

Copy link
Contributor Author

@VeijoPesonen VeijoPesonen Jan 9, 2019

Choose a reason for hiding this comment

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

Should we test that socket is actually closed?

That is done on the next if-clause.

If the connection cannot be established or gets
disrupted during data transmission, the system
returns:
ERROR

From the documentation.

So the question is can there occur an ERROR without socket being closed...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah.. true.. Thanks.

Driver must return NSAPI_ERROR_WOULD_BLOCK if a server won't accept
data from the modem and the modem's buffers are full. In case that
socket is closed driver returns NSAPI_ERROR_CONNECTION_LOST.
@VeijoPesonen
Copy link
Contributor Author

Fixed my own review finding.

@cmonr
Copy link
Contributor

cmonr commented Jan 10, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Jan 10, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit e25611a into ARMmbed:master Jan 10, 2019
@VeijoPesonen VeijoPesonen deleted the bugfix-esp8266_send_busy branch January 10, 2019 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants