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

fix #2007 http: Call callback in all cases #2020

Merged
merged 5 commits into from
Jul 1, 2017

Conversation

HHHartmann
Copy link
Member

@HHHartmann HHHartmann commented Jun 30, 2017

Call callback with errorcode -1 if no connection could be establioshed

Fixes #2007 and #1721.

Make sure all boxes are checked (add x inside the brackets) when you submit your contribution, remove this sentence before doing so.

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/en/*.

the PR checks the returnvalue of espconn_(secure_)disconnect. If no connection was established it fails and no disconnected callback is to be expected.
In this case the callback is called directly

I allso added some more error logging which I can remove on request.

Call callback with errorcode -1 if no connection could be establioshed
return;
}
request_args_t * req = (request_args_t *) conn->reverse;
HTTPCLIENT_ERR( "Calling disconnect" );
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be HTTPCLIENT_DEBUG?

@marcelstoer marcelstoer added this to the 2.1 follow-up milestone Jul 1, 2017
@marcelstoer
Copy link
Member

marcelstoer commented Jul 1, 2017

Thank you. In #2007 we established that there's also an inconsistency in our documentation wrt the disconnection handling. Can I ask you to clean this up as well in this PR?

return;
}
if ( conn->reverse == NULL )
{
HTTPCLIENT_DEBUG( "reverse is NULL" );
Copy link
Member

Choose a reason for hiding this comment

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

It may be helpful to print a more explanatory message. The meaning of "reverse" may not be totally obvious.

Copy link
Member Author

Choose a reason for hiding this comment

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

@marcelstoer As to the documentation inconsistency: I am not aware of it, please enlighten me so I can add it.

The fix above establishes the behavior as in the docs AFAICS.

note errorhandling on every call instead of only in the main documentation
@HHHartmann
Copy link
Member Author

@marcelstoer Documentation is a good point, already forgot about that problem. But there you go ...

@marcelstoer marcelstoer merged commit 15b4fa2 into nodemcu:dev Jul 1, 2017
@HHHartmann
Copy link
Member Author

@marcelstoer well I intentionally left out the period as it had been omitted in the other paragraphs as well ;-)

eiselekd pushed a commit to eiselekd/nodemcu-firmware that referenced this pull request Jan 7, 2018
* fix 2007 Call callback in all cases, call callback with errorcode -1 if no connection could be establioshed
* change logging from ERR to DEBUG
* make debug output more clear (hopefully)
* add handling of errors to docs, note error handling on every call instead of only in the main documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants