Skip to content

Align prototype & implementation of enet_tasklet_disconnect & friends #9100

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

Merged
merged 1 commit into from
Dec 17, 2018

Conversation

deece
Copy link
Contributor

@deece deece commented Dec 14, 2018

Description

Rework of #8698

Aligns the prototype and implementation of enet_tasklet_disconnect.

Pull request type

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

@deece
Copy link
Contributor Author

deece commented Dec 14, 2018

Original fix 7e7af77 coerces an int8_t to nsapi_error_t, which is incorrect (nsapi_error_t is an enum with specific values)

@ciarmcom
Copy link
Member

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

@@ -97,5 +97,6 @@ nsapi_error_t NanostackEthernetInterface::do_initialize()

nsapi_error_t Nanostack::EthernetInterface::bringdown()
{
return enet_tasklet_disconnect(true);
(void)enet_tasklet_disconnect(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be turning an error into some sort of nsapi error code, rather than swallowing it. I guess DEVICE_ERROR would do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did contemplate that (it does make sense), but the original code swallowed it, and without a way to test it, I didn't think it would be good to alter the behavior.

@@ -20,6 +20,7 @@
#include "net_interface.h"
#include "ip6string.h" //ip6tos
#include "nsdynmemLIB.h"
#include "include/enet_tasklet.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

There is already an include of enet_tasklet.h below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bad merge :) I'll remove it

@cmonr
Copy link
Contributor

cmonr commented Dec 14, 2018

@SeppoTakalo Please take a look. Should be similar to #8698, which was OK'd before.

@cmonr
Copy link
Contributor

cmonr commented Dec 14, 2018

CI started

@mbed-ci
Copy link

mbed-ci commented Dec 15, 2018

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter

@cmonr cmonr merged commit c7c24de into ARMmbed:master Dec 17, 2018
@deece deece deleted the fix-8695 branch December 17, 2018 21:48
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.

6 participants