-
Notifications
You must be signed in to change notification settings - Fork 294
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
rpmsg: wait endpoint ready in rpmsg_send #238
base: main
Are you sure you want to change the base?
Conversation
data, len, true); | ||
metal_sleep_usec(RPMSG_TICKS_PER_INTERVAL); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As previously discussed this imposes a way of implementing to other platforms which would not want to wait on first message, and not be blocked because channel has been closed by remote side (RPMSG_NS_DESTROY).
Look to me that test in rpmsg_send_offchannel_raw is enough to protect from a non initialized destination address .
The loop should be implement in application if necessary, to keep flexibility in the lib usage.
@edmooring
Any opinion on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this change doesn't forbid the user do the loop in application, user has all possible choice:
- Call rpmsg_try_send and check the return value
- Check dest_addr before call rpmsg_send
- Let rpmsg_send to do the check
With this patch, user can still do item 1 and item 2 as they want, but item 3 can protect user if they forget to do the check.
BTW, it also follow the same behaviour when the vring buffer exhaust:
https://github.com/OpenAMP/open-amp/blob/master/lib/rpmsg/rpmsg_virtio.c#L305
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Arnaud. This changes the behavior of an existing API in a way that could be very surprising to a user by introducing a possible 15 second delay in their application.
This should be done in an application, where the user knows what conditions they might be expected and can set timeout values and retry patterns in a way that is best for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let's see how many user will report this issue and then decide whether a common solution is needed.
data, len, true); | ||
metal_sleep_usec(RPMSG_TICKS_PER_INTERVAL); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Arnaud. This changes the behavior of an existing API in a way that could be very surprising to a user by introducing a possible 15 second delay in their application.
This should be done in an application, where the user knows what conditions they might be expected and can set timeout values and retry patterns in a way that is best for them.
c003f1e
to
23ab7b2
Compare
23ab7b2
to
e0b944a
Compare
e0b944a
to
34a5576
Compare
34a5576
to
b2cb1e0
Compare
b2cb1e0
to
2015b30
Compare
e676545
to
8571a7e
Compare
This pull request has been marked as a stale pull request because it has been open (more than) 45 days with no activity. |
8571a7e
to
5e85907
Compare
because the remote need time to return the destination address Signed-off-by: Guiding Li <[email protected]>
5e85907
to
4c3b211
Compare
because the remote need time to return the destination address