-
Notifications
You must be signed in to change notification settings - Fork 914
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
Allowed saving timestamp-less messages to Cache (using rospy.Time.now() as their timestamp). #636
Conversation
…() as their timestamp). Also added a convenience `getLast` method for retrieving the last message in the cache.
Can one of the admins verify this patch? |
I don't believe we should ever automatically stamp unstamped data. An alternative would be to provide a 2nd argument to which the user could add the timestamp explicitly. |
Okay, what about adding an |
That constructor argument sounds like a good option. |
return [m for m in self.cache_msgs | ||
if m.header.stamp >= from_stamp and m.header.stamp <= to_stamp] | ||
return [self.cache_msgs[i] for i in range(0, len(self.cache_msgs)) | ||
if self.cache_times[i] >= from_stamp and self.cache_times[i] <= to_stamp] |
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.
Please keep the list comprehension as simple as possible, e.g.:
[ m for i, m in enumerate(self.cache_msgs) if self.cache_times[i] >= from_stamp and self.cache_times[i] <= to_stamp]
Or even better you could even use zip
to iterate over both lists at the same time.
…explicitly asked to in contructor.
I've implemented your suggestion with the constructor argument. I've also added tests. Should I retarget to kinetic-devel, or is it okay this way? |
lgtm, it would be better to target for kinetic and consider backporting only if there's strong demand |
Ok, see #806 . |
Also added a convenience
getLast
method for retrieving the last message in the cache.