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

Use SBCL Unix API to get the timestamp instead of CL functions. #24

Merged
merged 3 commits into from
Mar 6, 2015
Merged

Use SBCL Unix API to get the timestamp instead of CL functions. #24

merged 3 commits into from
Mar 6, 2015

Conversation

gaya-
Copy link
Member

@gaya- gaya- commented Feb 18, 2015

The main change is line 59 - 63. Everything else is minor indentation & co. fixes.

I was getting 0.6s difference on my system, which is pretty substantial...

@gaya-
Copy link
Member Author

gaya- commented Feb 18, 2015

Within roslisp the two special variables *time-base* and *internal-time-base* are only used for this one function. I would get rid of them but someone from the outside might be using them even though they're not exported from the :roslisp namespace. Not sure how you best deprecate variables...

@daewok
Copy link
Contributor

daewok commented Feb 18, 2015

I've seen variable deprecations done with symbol macros before (can't remember where, though). Something like this would probably work:

(defvar *%time-base* (unix-time)
  "(Deprecated) Holds unix time (rounded to the nearest second) when roslisp was started")
(define-symbol-macro *time-base* (progn (ros-warn (roslisp time) "*time-base* is deprecated.") *%time-base*))

According to the Hyperspec though, the behavior of this will be undefined if a function has *time-base* declared as special.

@gaya-
Copy link
Member Author

gaya- commented Feb 27, 2015

@daewok, thanks!

According to the Hyperspec though, the behavior of this will be undefined if a function has time-base declared as special.

As (declare (special *my-var*)) only makes sense inside of a lexical scope where *my-var* is the local variable name and one wants to turn it from a lexical variable to special variable, I don't see a point of using a name of an already existing special variable here. Especially, if that variable belongs to a different package. E.g. I can't see anyone writing (defun bla (roslisp::*time-base*) ...). Except maybe when the code is (in-package :roslisp), but if one goes so far as to mess with the internal roslisp implementation they might as well fix their variable names. Otherwise, SBCL will throw a compilation error, which isn't bad IMO.

@airballking
Copy link
Contributor

I ran a quick sanity-test. It all compiles, runs, and makes sense to me.

Here is what I did:

  • use_sim_time is false
  • $ roscore on my local machine
  • $ rosrun tf static_transform_publisher 1 2 3 0 0 0 a b 100 to have some publisher with a timestamp. NOTE: this is dated into the future by 100ms
  • in the repl:
    • load roslisp and tf2_msgs
    • start a roslisp-node
    • define a callback function:
(defun from-msg (msg)
  (let ((now (ros-time)))
    (with-fields (transforms) msg
      (with-fields ((then (stamp header))) (elt transforms 0)
        (format t "~%now: ~a then: ~a diff: ~a~%" now then (- now then)))))
  • subscribe: (defparameter *sub* (subscribe "/tf" "tf2_msgs/TFMessage" #'from-msg))

This prints something like this:
now: 1.42556563643949d9 stamp: 1.4255656365391152d9 diff: -0.0996251106262207d0

If I ask the static_transform_publisher to run with a period of 50ms I get:
now: 1.425566523690594d9 then: 1.4255665237398553d9 diff: -0.04926133155822754d0

So, I get what I expected: Almost the diff by which the static_transform_publisher future-dates. Cool! I tried this 3 times with recompiling and restarting. All fine.

I'd like to merge. @gaya- @moesenle Any final thoughts?

EDIT: If you wonder about the future-dating, it happens here:
https://github.com/ros/geometry/blob/indigo-devel/tf/src/static_transform_publisher.cpp#L72

airballking added a commit that referenced this pull request Mar 6, 2015
Use SBCL Unix API to get the timestamp instead of CL functions.
@airballking airballking merged commit 03c1efa into ros:master Mar 6, 2015
@gaya- gaya- deleted the time-function-fix branch July 9, 2015 14:11
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