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

Enable to call (one-shot-subscribe) safely in (let ...) scope #667

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

708yamaguchi
Copy link
Member

@708yamaguchi 708yamaguchi commented May 20, 2021

For detail, please see #666

There are several cases where (one-shot-subscribe) occurs an error.

1. (one-shot-subscribe ...) -> C-c -> reset -> (let (a) (one-shot-subscribe ...)) ;; Bug
2. (one-shot-subscribe ... :unsubscribe nil) -> (let (a) (one-shot-subscribe ...)) ;; Bug
3. (one-shot-subscribe ...) -> C-c -> (one-shot-subscribe ...) ;; Bug
4. (ros::subscribe topic ...) -> (one-shot-subscribe topic) -> Bug
5. one-shot-subscribe -> C-c -> reset -> (ros::spin-once) -> Endless loop if a message is received
6. one-shot-subscribe -> C-c -> reset -> (let (a) (ros::spin-once)) -> Segmentation Fault if a message is received

In this PR,

  • I solve problem 1. ~ 4. by removing :unsubscribe argument and checking subsciber. This makes sure that there is no subscriber before (while ... (ros::spin-once)) is called. (the first commit of this PR)
  • I solve problem 5. ~ 6. by using class closures for callback function of (one-shot-subscribe). This allows the callback function to be accessed safely from anyscope. (the second commit of this PR)

My concern is that by removing the :unsubscribe argument, backward compatibility will be lost.

Thank you very much for your help.
@YoheiKakiuchi @pazeshun @Affonso-Gui

@Affonso-Gui
Copy link
Member

Affonso-Gui commented May 20, 2021

It would also be nice to have some more debate and some other points of view on the changes below:

  • Removing the :unsubscribe argument.
    I am kind of comfortable with this change since I couldn't find any occurrences in the jsk_robot and pr2eus, but I'm not sure about hrpsys and tendon robots. If backwards compability is a must I guess we could also keep the key arg and just print a "deprecated" warning and unsubscribe it anyway?

  • How to reject calls if prior subscribers are found.
    Most of the roseus functions log errors and return nil, but I think in this case it might be good to raise standard euslisp errors as well, since the program is most likely to briefly fail with something like cannot find method :... in ... when unsuccessful.

In some other minor comments:

  • How about using ros::get-topic-subscriber instead of ros::get-num-publishers? This seems a lot more intuitive to me.
  • The name of the second commit says "pointer closures" but I'm guessing you mean class closures?
  • Some part of me would like to see the helper class as the internal symbol of some other package or with some underbar-ish crazy prefix that makes it very clear that it does not need to be directly invoked by the user.

@708yamaguchi
Copy link
Member Author

Thank you very much for your review.
I updated the code and pushed the commits.

I too would like to ask other people's ideas.

@Affonso-Gui
Copy link
Member

@tkmtnt7000

Copy link
Member

@Affonso-Gui Affonso-Gui left a comment

Choose a reason for hiding this comment

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

So thinking a little bit more about the subject, I think that it should be ok to remove the :unsubscribe argument and to log warnings without raising any explicit euslisp errors.

However, I would still like to see ros::get-topic-subscriber instead of ros::get-num-publishers and a better name for the auxiliary class.

@knorth55 knorth55 mentioned this pull request Jul 6, 2022
3 tasks
Copy link
Member

@Affonso-Gui Affonso-Gui left a comment

Choose a reason for hiding this comment

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

Revisiting this topic in #725

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