Check for OOBs on background #106
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
VeijoPesonen
requested review from
SeppoTakalo,
kjbracey,
marcuschangarm and
michalpasztamobica
SeppoTakalo
suggested changes
Oct 17, 2018
| char status[5]; | ||
| if (_parser.recv("%4[^\"]\n", status)) { | ||
| if (strcmp(status, " s...\n") == 0) { | ||
| ; //TODO maybe do something here, or not... |
There was a problem hiding this comment.
What is the whole reason for this OOB handler if it does not do anything?
There was a problem hiding this comment.
Just making people aware there are this kind of events which might be raised. And especially to report an error if some new firmware introduces new messages on which we are to react.
ESP8266/ESP8266.cpp
Outdated
| } | ||
| } else { | ||
| MBED_ERROR(MBED_MAKE_ERROR(MBED_MODULE_DRIVER, MBED_ERROR_CODE_EBADMSG), \ | ||
| "ESP8266::_oob_busy: busy doing what?\n"); |
There was a problem hiding this comment.
I don't see the value of this error message. Messages should be targeted to users. So don't ask users what the ESP is doing.
Instead report that ESP is reporting busy status.
There was a problem hiding this comment.
Could change to something like "unrecognized AT busy state"
There was a problem hiding this comment.
Yes, that would be more informative.
There was a problem hiding this comment.
Seems to be I have either bug on the code or there really is some unrecognized busy state as I was able to trigger one from those two code paths - unintentionally:
[1539777313.37][CONN][RXD] ++ MbedOS Error Info ++
[1539777313.41][CONN][RXD] Error Status: 0x8008004A Code: 74 Module: 8
[1539777313.47][CONN][RXD] Error Message: ESP8266::_oob_busy: busy doing what?
[1539777313.48][CONN][RXD]
[1539777313.49][CONN][RXD] Location: 0x7FF1
[1539777313.51][CONN][RXD] Error Value: 0x0
[1539777313.61][CONN][RXD] Current Thread: Id: 0x2000434C Entry: 0x14607 StackSize: 0x1000 StackMem: 0x20004390 SP: 0x20005048
[1539777313.70][CONN][RXD] For more info, visit: https://armmbed.github.io/mbedos-error/?error=0x8008004A
[1539777313.72][CONN][RXD] -- MbedOS Error Info --
|
Change the processing to use the global event queue (after you have got rid of 2s timeout). |
added 5 commits
October 17, 2018 15:34
VeijoPesonen
force-pushed
the
feature-bg-network_status_thread
branch
from
e1a7f83 to
7b978e9
Compare
|
@SeppoTakalo, @kjbracey-arm , @marcuschangarm , @michalpasztamobica please review. Background processing changed to use global event queue instead of dedicated thread. |
ESP8266Interface.h
Outdated
|
|
||
| #include "events/EventQueue.h" | ||
| #include "events/mbed_shared_queues.h" | ||
| #include "rtos/Thread.h" |
There was a problem hiding this comment.
rtos/Thread.h is no longer needed when using the event queue, right?
SeppoTakalo
reviewed
Oct 25, 2018
ESP8266Interface.cpp
Outdated
| @@ -104,6 +118,17 @@ int ESP8266Interface::connect(const char *ssid, const char *pass, nsapi_security | |||
| return connect(); | |||
| } | |||
|
|
|||
| void ESP8266Interface::_oob2geq() | |||
There was a problem hiding this comment.
geq?
As in.. global_event_queue?
SeppoTakalo
suggested changes
Oct 25, 2018
README.md
Outdated
| @@ -52,6 +52,30 @@ Least one is expected to check are the following configuration parameters | |||
| } | |||
| ``` | |||
|
|
|||
| To enable network status updates from the driver one must take Global EventQueue into use via app config file. Otherwise | |||
There was a problem hiding this comment.
This is not true..
mbed_event_queue(); will spawn a thread UNLESS "events.shared-dispatch-from-application": true
So this setting is just a possible memory optimization..
There was a problem hiding this comment.
Maybe we should remove this whole section.. Let the event queue spawn its own thread.. allow those who try to optimize memory to study this from Mbed OS documentation.
There was a problem hiding this comment.
Section dropped.
README.md
Outdated
| "esp8266.cts": "NC" | ||
| "esp8266.rx": "D0", | ||
| "esp8266.tx": "D1", | ||
| // Adds dispatch_forever() to your main-function |
There was a problem hiding this comment.
It does not add.. it requires
There was a problem hiding this comment.
Section dropped.
SeppoTakalo
approved these changes
Oct 25, 2018
|
Looks good now.. Have you got any test results after this change? |
|
There is need to increase the stack size for the thread spawned for global event queue. Unless this is done in app config stack overflow occurs with our netsocket test cases
|
Test Results - AT firmware version 1.6.2 without HW flow controlnetsocket-tcpnetsocket-udpnetwork-interface |
Test Results - AT firmware version 1.7.0 with HW flow controlnetsocket-tcpTake a note how much longer it takes to execute the TCP test cases with newer firmware/my driver changes. I should address those issues in another PR. netsocket-udpnetwork-interface |
VeijoPesonen
force-pushed
the
feature-bg-network_status_thread
branch
from
dce922a to
d1d84ee
Compare
|
@SeppoTakalo fixup commits squashed. Do you agree that those performance issues with the newer firmware can be addressed in another PR? |
|
The change kind of looks good, but the performance is horrible. 10x slower on some test cases. |
|
The performance of the driver for the newer firmware hasn't ever been what it is supposed to be. The undersigned is to be blamed for that but corrective actions are what I'm going to be working with next. |
|
Man, you weren't kidding about that performance degradation! 😨 |
|
@marcuschangarm PR ARMmbed/mbed-os#8598 should improve the situation. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Check for OOBs on background. Spawns a thread for that purpose.
Pull request type