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

nsapi: Rename attach -> sigio to decrease confusion on its behaviour #3784

Merged
merged 1 commit into from
Feb 21, 2017

Conversation

geky
Copy link
Contributor

@geky geky commented Feb 15, 2017

Currently the Socket::attach function behaves differently than other attach functions in the mbed codebase.

There is quite a bit of discussion on the future of asynch socket apis (related issue #3620), and given the current focus of mbed OS, I think it will take some time before we see a full-featured async user api mainlined.

In the meantime, I would like to change the name of the function to hopefully 1: reduce some of the confusion around it and 2: free up the attach name to potentially be used for a full-fledge async api in the future:

/** Register a callback on state change of the socket
 *
 *  The specified callback will be called on state changes such as when
 *  the socket can recv/send/accept successfully and on when an error
 *  occurs. The callback may also be called spuriously without reason.
 *
 *  The callback may be called in an interrupt context and should not
 *  perform expensive operations such as recv/send calls.
 *
 *  Note! This is not intended as a replacement for a poll or attach-like
 *  asynchronous api, but rather as a building block for constructing
 *  such functionality. The exact timing of when the registered function
 *  is called is not garunteed and susceptible to change.
 *
 *  @param func     Function to call on state change
 */
void sigio(mbed::Callback<void()> func);

cc @kjbracey-arm, @hasnainvirk, @marcuschangarm, @bogdanm

Copy link
Contributor

@kjbracey kjbracey left a comment

Choose a reason for hiding this comment

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

Agree with the change - just comment nits.

Don't understand the observation "It doesn't help that the Socket::attach function's behaviour isn't really intended to be user consumable at this point."

How is it not intended? Sockets are user API, and SIGIO is the standard socket wake-up mechanism. If Sockets are user consumable, their wake-up mechanism has to be.

* Note! This is not intended as a replacement for a poll or attach-like
* asynchronous api, but rather as a building block for constructing
* such functionality. The exact timing of when the registered function
* is called is not garunteed and susceptible to change.
Copy link
Contributor

Choose a reason for hiding this comment

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

garunteed -> guaranteed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ackkk 👍

* @param func Function to call on state change
* @deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems misleading to have a whole duplicate comment block (to me at least). Implies it somehow might have slightly different functionality from sigio, and we need to compare the comments to spot the difference.

Can't you just have some sort of reference link?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could avoid documenting deprecated functions and just use @see sigio to refer people to what they should use instead in doxygen.

Copy link
Contributor Author

@geky geky Feb 16, 2017

Choose a reason for hiding this comment

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

Ah, @bulislaw, that's a good idea, I'll drop the description for just the @see and @deprecated notes.

I think we need to keep the deprecated notice in doxygen, the otherwise it won't show up in the online render of the doxygen.

Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

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

+1, I don't generally see attach used across mbed as something that always makes things better or less confusing, in some cases it makes sense, but often other names would be more descriptive.

* @param func Function to call on state change
* @deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could avoid documenting deprecated functions and just use @see sigio to refer people to what they should use instead in doxygen.

@geky
Copy link
Contributor Author

geky commented Feb 16, 2017

@kjbracey-arm, hmm, your point is valid. I just would prefer users avoid sigio unless they understand how it is intended to be used.

Is there any wording you would like to see different in the doxygen?


@bulislaw, I generally agree, although it does seem users are currently comfortable with what it means in the drivers. Perhaps some day it would be interesting to adopt irq/event functions. (This would also take care of the nasty habit of users accidentally ending up in irq context.)

@kjbracey
Copy link
Contributor

I'm happy with the doxygen.

I would also prefer users avoid sigio unless they understand how it is intended to be used. But that's just a general statement which could apply to any API - I don't see sigio as exceptional in anyway.

@bridadan
Copy link
Contributor

/morph test-nightly

@sg- sg- removed the needs: review label Feb 17, 2017
@sg-
Copy link
Contributor

sg- commented Feb 17, 2017

/morph test-nightly

@mbed-bot
Copy link

Result: ABORTED

Your command has finished executing! Here's what you wrote!

/morph test-nightly

@bridadan
Copy link
Contributor

There was a double-start on CI, so nobody freak out about the red X 😄 Please don't restart the CI for now! When the first job is done it should mark the PR as green (assuming it passes).

@mbed-bot
Copy link

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1626

Test failed!

@geky
Copy link
Contributor Author

geky commented Feb 17, 2017

Ah! It's this issue: #3779
/morph test-nightly

@mbed-bot
Copy link

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1637

All builds and test passed!

@sg- sg- merged commit 6f6e5c4 into ARMmbed:master Feb 21, 2017
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.

7 participants