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

openamp: add ns_unbind_cb to rpmsg_device #376

Merged
merged 1 commit into from
May 16, 2022

Conversation

xiaoxiang781216
Copy link
Collaborator

@xiaoxiang781216 xiaoxiang781216 commented Apr 18, 2022

it's useful to notify the port layer and also symmetry with ns_bind_cb callback.

@arnopo
Copy link
Collaborator

arnopo commented Apr 19, 2022

Could you detail your need? in which use case you need to notify the port layer in addition of the endpoint?

@xiaoxiang781216
Copy link
Collaborator Author

Could you detail your need? in which use case you need to notify the port layer in addition of the endpoint?

In some case, the service/apps may start later than open-amp framework, and then lose the creation notification from remote peer. To fix this issue, the porting layer could catch the notification and forward it to the client/service at the launch.
Since the client/server may create and destroy the endpointer dynamically and unbound, the porting layer need some mechanism(unbind callback here) to reclaim the resource from cache.

lib/rpmsg/rpmsg_virtio.c Show resolved Hide resolved
@arnopo
Copy link
Collaborator

arnopo commented May 4, 2022

Could you detail your need? in which use case you need to notify the port layer in addition of the endpoint?

In some case, the service/apps may start later than open-amp framework, and then lose the creation notification from remote peer. To fix this issue, the porting layer could catch the notification and forward it to the client/service at the launch.
Since the client/server may create and destroy the endpointer dynamically and unbound, the porting layer need some mechanism(unbind callback here) to reclaim the resource from cache.

In such case, is the endpoint created by the porting layer?
Seems quite tricky to address:

  • your port layer could free resource that are still used by the application.
  • you have to manage error case where an unbind is receive after the bind, while the application is not ready.

What about having the endpoint->ns_unbind_cb calling a porting layer function that would forward to the application?

@arnopo arnopo requested review from arnopo and edmooring May 4, 2022 16:18
@xiaoxiang781216
Copy link
Collaborator Author

xiaoxiang781216 commented May 4, 2022

Could you detail your need? in which use case you need to notify the port layer in addition of the endpoint?

In some case, the service/apps may start later than open-amp framework, and then lose the creation notification from remote peer. To fix this issue, the porting layer could catch the notification and forward it to the client/service at the launch.
Since the client/server may create and destroy the endpointer dynamically and unbound, the porting layer need some mechanism(unbind callback here) to reclaim the resource from cache.

In such case, is the endpoint created by the porting layer?

No, the endpoint is still created by service/application. The resource I mean here is simple channel name and port number:
https://github.com/apache/incubator-nuttx/blob/master/drivers/rptun/rptun.c#L581-L582

Seems quite tricky to address:

  • your port layer could free resource that are still used by the application.

No, the house keeping data is managed by the porting layer internally, not shared with application at all.

  • you have to manage error case where an unbind is receive after the bind, while the application is not ready.

Of course, the application doesn't get any notification in this case since the channel name and port number is removed from cache in ns_unbind_cb callback.

What about having the endpoint->ns_unbind_cb calling a porting layer function that would forward to the application?

As I explain before, the porting layer doesn't create the endpoint behalf the application, so it can't hook the notification from endpoint->ns_unbind_cb.

BTW, it's also wrong to create the endpoint in porting layer because:

  1. The service represented by the endpoint mayn't exist at all. It's hard to know inside the porting layer in a dynamical environment.
  2. Even the service exist, it's wrong to create the endpoint before the service is ready. e.g. How to handle the message send from client before the service start?

Copy link
Collaborator

@arnopo arnopo left a comment

Choose a reason for hiding this comment

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

Thanks for the link to the code, easier to understand your need.
The addition of this callback seems reasonable to me.
just documentation to address

lib/include/openamp/rpmsg.h Show resolved Hide resolved
lib/rpmsg/rpmsg_virtio.c Show resolved Hide resolved
@xiaoxiang781216 xiaoxiang781216 force-pushed the unbind branch 2 times, most recently from fb557b2 to eaf2d77 Compare May 9, 2022 03:49
@xiaoxiang781216 xiaoxiang781216 requested review from arnopo May 10, 2022 08:46
it's useful to notify the port layer and
also symmetry with ns_bind_cb callback

Signed-off-by: Guiding Li <[email protected]>
@arnopo arnopo added this to the Release V2022.10 milestone May 13, 2022
Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

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

This seems like a sensible addition.

@arnopo arnopo merged commit 9fdadd0 into OpenAMP:main May 16, 2022
@xiaoxiang781216 xiaoxiang781216 deleted the unbind branch May 16, 2022 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants