-
Notifications
You must be signed in to change notification settings - Fork 101
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
Variable scan frequence #26
base: master
Are you sure you want to change the base?
Variable scan frequence #26
Conversation
Any reason why this PR is not merged yet? |
FYI @servos @mikepurvis |
Can someone explain a bit more about what this is doing? It looks like we're adding a new function which isn't actually called from anywhere. Can this behaviour be exposed through a parameter to the supplied node? |
For the pull request you are right; the function is never called.
|
@@ -294,6 +294,16 @@ class LMS1xx { | |||
void setScanDataCfg(const scanDataCfg &cfg); | |||
|
|||
/*! | |||
* @brief Set output range configuration. | |||
* Get output range configuration : |
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.
Set
I agree with not forcing a frequency and allowing to configure it from this driver. However, the configure method should be tested and exercised somewhere. IMHO it's buggy. See my comments. BTW, I don't know why this branch has conflicts... I guess it must be rebase on top of the main branch. |
@Karsten1987 , do you have time and the resources to fix and test this? |
FYI @asaini89 |
Sorry for reacting late on this. In fact I have a launch file plus small executable providing a ROS configuration node. I will add it soon. Have a look here https://github.com/Karsten1987/LMS1xx/commits/master @hanshammel1337 Your thinking is absolutely correct. I think it's necessary to provide functions like this to configure your lasers according to your application. BTW, most of the functions are applicable also for the TiM series. @efernandez I am not having access to a laser until Monday and will test the functions again according to your comments. I basically played monkey-see-monkey-do with the already existing functions. As for the merge conflicts, my guess would be white-spacing since the original code is using real tabs and I spaces. |
Karsten - have you had success with 25Hz 0.25 degree scan configurations on an LMS100? For whatever reason, if I disable the scan frequency check, enable 0.25 degree scan resolution in SOPAS, then start the node - it starts and connects successfully but then crashes the moment anything subscribes to the /scan topic. 25 Hz/0.5 degree works fine |
Given that I don't have access to any LMS anymore, please treat my answer here with caution. you may want to verify that your cfg-structs which you get from the driver actually correspond to the settings in SOPA. As far as I recall, I only tested 50Hz/0.5 deg and 25Hz/0.25deg. I am not sure if a mix is supported. |
Thanks for the info. I actually did figure out the issue about an hour after I posted, but in general you seem to be right. What I'm seeing is that in some cases, the LMS100 outright lies. Specifically, if I set 25Hz/0.25deg, the scan config returns the correct resolution value, but the scan output range query reports 0.5deg resolution when in fact the data is at 0.25deg resolution. As a result, the ROS node assumes half as many points as are actually being delivered, which leads to the crash. I changed LMS1xx_node.cpp to ignore the angular resolution information from getScanOutputRange() and only use the resolution from getScanConfig() and that seems to be working. Interestingly - at least on my LMS100, SOPAS does not allow you to configure the output data resolution - the only resolution config is the scan config. (May be a sign that I need to update firmware in the unit and/or sort out network access for the machine I'm running SOPAS on so I can use a DDS other than the one downloaded from the unit itself.) |
According to SICK's developer guide we have to trigger 2 functions to fully configure the laser.
The
LMS1xx::setScanCfg
LMS1.xx#L287 function alone does not sufficiently set the output range parameters. Hence the extension in this PR.