#347 sicklms200 read() bug fix

closed
Player (393)
5
2008-10-07
2008-09-29
No

Fixed read() in sicklms200 to handle errno returns. Currently, if errno's are seen, the bytes counter gets DECREMENTED, which leads to timeouts and sometimes CRC failures.

By checking for this case, the driver starts up better than I've ever seen it, even on new, fast kernels, where the LMS device cannot post fast enough to read things off continuously.

Discussion

  • Patrick Beeson

    Patrick Beeson - 2008-10-01

    I found another bug in the first read() of ReadFromLaser(). Here there is no "bytes+=" problem, but the buffer gets shifter after each read, even if it was not successful. Easy fix.

    New patch subsumes old patch.
    File Added: laser_read.v2.patch

     
  • Patrick Beeson

    Patrick Beeson - 2008-10-01

    Finalized my changes to the SICK driver. Here is a list of the fixes:

    1) Fixed read() errors. This gets rid of MANY timeouts and CRC errors (this replaces previously uploaded patch).
    2) Added high availability flag (high_avail) to set device in a mode to not reset when it gets dazzled by sunlight (instead it returns max readings).
    3) No longer blindly updates the resolution (the width and angular resolution of the scan) and/or the configuration (intensities, high availability, measurement distance) when the lasers values are equal to the desired values.
    4) Also increased one timeout slightly on the first command that is sent.

    File Added: laser_read.v3.patch

     
  • Patrick Beeson

    Patrick Beeson - 2008-10-01
    • assigned_to: gerkey --> thjc
     
  • Patrick Beeson

    Patrick Beeson - 2008-10-01

    I just noticed that Toby did some work on this driver that I was not aware of -- in player-2-1-patches revision 6992.

    However, part of his fixes are not going to work on newer kernels. Specifically, he did notice that the read()s were not looking for -1 (errors), but instead of just ignoring them, he immediately returns when he sees this -- not a bad thing to do if you don't know what else do to. However, newer kernels run so much faster than the clock on the LMS internal processor, that you actually see the 'Device not ready or resource busy' errno A LOT -- even in the middle of reading a long message. So falling out here is not the right thing to do. My current patch simply ignores the errors, but it may be best in the future to ignore the errors that occur due to newer kernels, while stopping and returning after other error messages.

     
  • Patrick Beeson

    Patrick Beeson - 2008-10-01

    On Toby's 6992 patch: maybe I'm wrong. I have never used select(), so it looks like that might cover the case where this will wait while the device is not ready, and then continue when the device puts something out there or when the timeout is met. I'm no longer at my robot, so I'll test Toby's better timeout/read() fix later.

    However, it turns out that by applying my patch, it fails on the read() fixes that Toby had already addressed while still keeping the other 3 things in the patch (see below for the list of 4 things the patch does).

    Toby, maybe you can explain to me why select needs to look a laser_fd+1 instead of laser_fd ? Thanks.

     
  • Patrick Beeson

    Patrick Beeson - 2008-10-03

    for trunk and player 2.1.1 release. 'patch -p0 < laser_read.v4.patch' works in server/drivers/laser

     
  • Patrick Beeson

    Patrick Beeson - 2008-10-03

    Last update I swear!

    I updated this patch to incorporate Toby's read() fixes that were in the release-2-1-patches branch. His fix for the read() bug was better than mine as it got rid of the existing usleeps to make the timeouts better.

    However, this patch still incorporates the four items listed in earlier comments.

    laser_read.v4.patch is made for the current trunk and player 2.1 release -- it adds Toby's read() fix.

    laser_read_B.v4.patch is made to be applied on top of Toby's fix in release-2-1-patches

    File Added: laser_read.v4.patch

     
  • Patrick Beeson

    Patrick Beeson - 2008-10-03

    File Added: laser_read_B.v4.patch

     
  • Kevin Barry

    Kevin Barry - 2008-10-03

    The latest patch adds an include for <laser_config.h>, which I don't have (2-1-patches).
    Also it changes SickLMS200_Register to sicklms200_Register.
    I imagine both of these come from 2.2 ?

    Changing those two things worked and the driver is usable (Tested on Mac OS X/LMS291).

    Thanks for your work on this driver. My SICK's flash memory thanks you, and the high_avail is great too.

    (It seems I cannot add an attachment to your post. The changes are minor anyway, but here is a patch against 2-1-patches: http://barryk.googlepages.com/laser_read_B.v4.1_KB.patch\)

     
  • Toby Collett

    Toby Collett - 2008-10-07
    • status: open --> closed
     

Log in to post a comment.