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

[jsk_robot_startup/smach_to_mail] Set sender address and receiver address from rosparam #71

Merged
merged 6 commits into from
Aug 26, 2022

Conversation

tkmtnt7000
Copy link
Collaborator

This PR enables to set mail address from rosparam.
Related to jsk-ros-pkg#1558

@@ -29,8 +29,8 @@ def __init__(self):
self.bridge = CvBridge()
self.smach_state_list = {} # for status list
self.smach_state_subject = {} # for status subject
self.sender_address = "[email protected]"
self.receiver_address = "[email protected]"
self.sender_address = rospy.get_param("~sender_address")
Copy link
Owner

Choose a reason for hiding this comment

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

@tkmtnt7000 thank you, please add error checking code, when no param is set, use default value or raise errors

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry for late.
I add error checking code(354bd5a)

@tkmtnt7000
Copy link
Collaborator Author

I think there might be some demand that we don't want to write their email address (robot or personal one) on github, so I make it possible to set the email address from the yaml file as well. The implementation is based on email_topic.py.

@k-okada
Copy link
Owner

k-okada commented Aug 23, 2022

@tkmtnt7000 pleaes update documents,
https://github.com/jsk-ros-pkg/jsk_robot/tree/master/doc/jsk_robot_common/jsk_robot_startup

I think there might be some demand that we don't want to write their email address (robot or personal one) on github, so I make it possible to set the email address from the yaml file as well. The implementation is based on email_topic.py.

In my opinion, we need to be very careful on using /var/lib,

  1. we put information on '/var/lib/robot' because we do not want to share these inforamtion on CVS
  2. Most of email address is already listed on package.xml.
  3. The more information we put in '/var/lib/robot', the more we want to put these information on CVS
T:今後この辺りもバージョン管理できたら履歴が残っていいかも、と思いました
Y:/var/lib以下ってこと?この辺りというのは
T:あ、そうです
  1. may be I sould not allow to use yaml file in Add ros node to send email based on received ros topic jsk-ros-pkg/jsk_robot#1388 ...
  2. If we want to use system-wide information, we should use rosparam, so that you can access from different machine.

--> Please discuss with @708yamaguchi and others, if we can remove /var/lib/robot/email_topic.yaml, please remove e58ac07, if that features is useful or already widely used, please simply update the document.

@tkmtnt7000
Copy link
Collaborator Author

@tkmtnt7000 pleaes update documents,
https://github.com/jsk-ros-pkg/jsk_robot/tree/master/doc/jsk_robot_common/jsk_robot_startup

OK. I'll write document about smach_to_mail.py overview first.

In my opinion, we need to be very careful on using /var/lib,

we put information on '/var/lib/robot' because we do not want to share these inforamtion on CVS
Most of email address is already listed on package.xml.
The more information we put in '/var/lib/robot', the more we want to put these information on CVS
T:今後この辺りもバージョン管理できたら履歴が残っていいかも、と思いました
Y:/var/lib以下ってこと?この辺りというのは
T:あ、そうです
may be I sould not allow to use yaml file in jsk-ros-pkg#1388 ...
If we want to use system-wide information, we should use rosparam, so that you can access from different machine.
--> Please discuss with @708yamaguchi and others, if we can remove /var/lib/robot/email_topic.yaml, please remove e58ac07, if that features is useful or already widely used, please simply update the document.

I understand. I will discuss with @708yamaguchi and other members whether to use /var/lib or not.
Surely, I also think that the more parts of the system that are not shared on CVS, the harder it is to find problems when they occur....

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.

3 participants