Mbed-os: API doesn't return NSAPI_ERROR_UNSUPPORTED on set_blocking(false)

[javiermelendezc]

Description

TARGET=CY8CPROTO_062_4343W
TOOLCHAIN=GCC_ARM

I was trying to implement the async API of Network Interface on the CY8CPROTO_062_4343W.

network = NetworkInterface::get_default_instance();
network->attach(&status_callback);
nsapi_error_t error = network->set_blocking(false);
switch (error)
{
case NSAPI_ERROR_OK:
printf("No Errors\r\n");
break;
case NSAPI_ERROR_UNSUPPORTED:
printf("Not supported\r\n");
break;
}
network->connect();
printf("This should be printed before any connection callbacks\r\n");

The asynchronous mode is never activated and it keeps handling connect() as a blocking function. Also, set_blocking(false) returns NSAPI_ERROR_OK.
Is this an error on the Cypress driver where NSAPI_ERROR_UNSUPPORTED should be returned from set_blocking(false)?

Issue request type

[ ] Question
[ ] Enhancement
[ X] Bug

[ciarmcom]

Internal Jira reference: https://jira.arm.com/browse/MBOCUSTRIA-1644

[0xc0170]

cc @ARMmbed/mbed-os-ipcore

[michalpasztamobica]

Hi, @javiermelendezc .
We have a greentea test prepared to test non-blocking connect.
Can you try to run it and let us know the results?
Also - can you provide the mbed_app.json and the commands you used to build your code?

[javiermelendezc]

I was trying to do the greentea test but didn't make it work due to python errors, will try later.
The code that I was running is plain simple:

Full code:

include "mbed.h"

include "nsapi_types.h"

NetworkInterface *network;

void status_callback(nsapi_event_t status, intptr_t param)
{
printf("Connection status changed!\r\n");
switch (param)
{
case NSAPI_STATUS_LOCAL_UP:
printf("Local IP address set!\r\n");
break;
case NSAPI_STATUS_GLOBAL_UP:
printf("Global IP address set!\r\n");
break;
case NSAPI_STATUS_DISCONNECTED:
printf("No connection to network!\r\n");
break;
case NSAPI_STATUS_CONNECTING:
printf("Connecting to network!\r\n");
break;
default:
printf("Not supported\r\n");
break;
}
}

int main(void)
{
network = NetworkInterface::get_default_instance();
network->attach(&status_callback);
nsapi_error_t error = network->set_blocking(true);
switch (error)
{
case NSAPI_ERROR_OK:
printf("No Errors\r\n");
break;
case NSAPI_ERROR_UNSUPPORTED:
printf("Not supported \r\n");
break;
}
network->connect();

printf("This should print before any WiFi callback\r\n");

}

mbed_app.json

{
"target_overrides": {
"*": {
"platform.stdio-convert-newlines": true,
"target.network-default-interface-type": "WIFI",
"nsapi.default-wifi-security": "WPA_WPA2",
"nsapi.default-wifi-ssid": "\"Impresora Z\"",
"nsapi.default-wifi-password": "\"imprezora\"",
"platform.stdio-baud-rate": 115200
}
}
}

Run using:

sudo mbed compile -t GCC_ARM -m CY8CPROTO_062_4343W -f

[michalpasztamobica]

Now, that you mentioned this is Wi-Fi - we are adding a test to explicitly check if WiFi enabled platforms handle non-blocking behavior correctly: https://github.com/ARMmbed/mbed-os/pull/11194 .
It seems most of them do not and I wouldn't be surprised if CY8CPROTO_062_4343W would be one of the majority.
Right now there is no dedicated emac driver for CY8PROTO_062_4343W and it is using the default EMACInterface implementation, which simply returns ERROR_OK. I quickly checked that this really gets called for NetworkInterface::get_default_instance()->set_blocking().
The _blocking variable later gets passed to the underlying layer and depends on it to handle things right.

Perhaps, the default EMACInterface works well enough to support the basic communication but to be 100% correct we should have a dedicated EMAC driver?

@ARMmbed/team-cypress , would you have any thoughts on this?

[javiermelendezc]

I think that the documentation may be misleading, specifically on the response handling of set_blocking(). Actually that's what led me to open the issue at first.

Returns
NSAPI_ERROR_OK on success
NSAPI_ERROR_UNSUPPORTED if driver does not support asynchronous mode.
negative error code on failure.

Shouldn't the default be NSAPI_ERROR_UNSUPPORTED on EMACInterface until the driver exist or implements the async mode in order to avoid this kind of confusion on future implementations of new devices?
Or it must be handled by the driver itself (in this case CY8PROTO_062_4343W) and they should take care of return NSAPI_ERROR_UNSUPPORTED in cases like this?

[0xc0170]

@ARMmbed/team-cypress , would you have any thoughts on this?

@ARMmbed/team-cypress please review

[cydriftcloud]

Since the WhdSTAInterface directly extend EMACInterface, which by default support the non-blocking mode. However, we in fact does NOT support this mode. Can you clarify why the EMACInterface by default turns this mode on? Does it has to be supported? If not, then we can override this function and indicate it is NOT supported by our interface

[javiermelendezc]

In my opinion the support of not blocking mode depends on each target implementation (it would be an awesome feature if Cypress support it tho). If not, the Cypress API must override set_blocking() and return ' NSAPI_ERROR_UNSUPPORTED' instead in order to avoid confusion.

I think that overriding the implementation of set_blocking() in EMACInterface (or demand the API developers to do it) to return ' NSAPI_ERROR_UNSUPPORTED' by default instead of 'NSAPI_ERROR_OK' must be a priority in order to avoid misleading on further implementations like this one.

Any thoughts @michalpasztamobica on this ?

[michalpasztamobica]

Yes, EMACInterface tries to support the non-blocking operation, mainly by passing the blocking flag to the underlying stack. If the stack is not supporting the blocking operation it has to override the set_blocking() function to return UNSUPPORTED error and not set the blocking flag (as it makes no sense really and is probably unused).

In my opinion the set_blocking() function is provided the way it is now for two reasons:
1) EMACInterface is not just an abstract interface to inherit from and it has a lot of "glue logic" in it. It can be and is instantiated. Most underlying stacks (LWIP, Nanostack) do support the non-blocking operation and need that flag.
2) For convenience - probably most stacks which do support non-blocking operation will just... set the blocking variable and return NSAPI_ERROR_OK 😄 (example)

@SeppoTakalo , please correct me if I failed to explain this the right way.

EDIT: I will also leave an indication in the documentation on how the set_blocking should behave, as that didn't get copied down into every inheriting class.

[cydriftcloud]

Thanks for the clear explanation. I will put up a PR to override the default set_blocking() to indicate not support.

[michalpasztamobica]

Hi @cydriftcloud , did you by any chance have time to try and modify the set_blocking() function?
@javiermelendezc , is the issue still relevant?

[ifyall]

@michalpasztamobica , PR https://github.com/ARMmbed/mbed-os/pull/11662 has been submitted to address this issue.

Thank you,

Ian

[michalpasztamobica]

@javiermelendezc , the fix is merged, would you like to try it out?

[maclobdell]

@javiermelendezc - please confirm if you are okay to close this issue now.

[javiermelendezc]

Sorry for the delay folks, had to order a new board to try it out.
It looks good, Thanks for the clear explanations on this one!