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

Feature/lidar lite #3

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

Conversation

rikba
Copy link
Contributor

@rikba rikba commented Dec 11, 2019

This PR introduces range sensors into the VersaVis repository.

Currently, we add our fork as LidarEnhanced dependency: https://github.com/rikba/LIDAREnhanced/tree/devel/versavis but if AlexisTM/LIDAREnhanced#13 and AlexisTM/LIDAREnhanced#12 are merged, we can also use the main repo.

@rikba
Copy link
Contributor Author

rikba commented Dec 11, 2019

Tested on Rev 1.1

header: 
  seq: 405
  stamp: 
    secs: 196551113
    nsecs:  80901000
  frame_id: "lidar_lite"
radiation_type: 1
field_of_view: 0.00800000037998
min_range: 0.0500000007451
max_range: 40.0
range: 1.90999996662

@rikba rikba mentioned this pull request Dec 11, 2019
Copy link
Contributor

@floriantschopp floriantschopp left a comment

Choose a reason for hiding this comment

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

Sorry for the VERY late review!
I think your timestamp setting logic is not right here, otherwise looks very nice :-)

@@ -4,3 +4,6 @@
[submodule "dependencies/versavis_hw"]
path = dependencies/versavis_hw
url = git@github.com:ethz-asl/versavis_hw.git
[submodule "firmware/libraries/LIDAREnhanced"]
path = firmware/libraries/LIDAREnhanced
url = git@github.com:rikba/LIDAREnhanced.git
Copy link
Contributor

Choose a reason for hiding this comment

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

both mentioned PRs got merged, we could switch to main repo

// Get range and trigger new measurement.
nack = lidar_controller_.distanceAndAsync(kId, &range_msg_.range);
// New measurement triggered. Set the next time stamp.
Sensor::setTimestampNow();
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to set the timestamp before writing it to the message header (Line 45) for this specific measurement (e.g. at the very beginning of publish or in triggerMeasurement). Like implemented here, the timestamp obtained here will be written in the message header the NEXT time the timer interrupts.

return;
}

if (range_msg_.time.data.sec != 0 && range_msg_.time.data.nsec != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is a catch for the very first measurement. If you adapt my suggestion above, this should not happen anymore.

@@ -0,0 +1,30 @@
<?xml version="1.0"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you rename this to run_lidar_lite_only?

<param name="range_sensitivity" value="0.01" />
<param name="min_signal_strength" value="0" /> <!-- No minimum signal strength -->
</node>

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment here to force-initialize the cameras.

~VersaVisRangeReceiver() { node_handle_.shutdown(); }

void rangeCallback(const versavis::RangeMicro &range_micro_msg) {
ROS_INFO_ONCE("Received first Range message from topic: %s",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ROS_INFO_ONCE("Received first Range message from topic: %s",
ROS_INFO_ONCE("Received first RangeMicro message from topic: %s",

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.

2 participants