Updates #43

shaun-createc · 2020-02-06T15:59:54Z

Better timing using the timestamp feature (if available)
Removed hard coded camera frame sizes in favor of asking the camera for them.
Added ability to open camera by user id feature (if available) and allowed driver to open first camera for one camera setups.
Updated PLUGINLIB macro to work on Melodic, breaks build on Groovy and earlier.
Added wrappers for the xiParam functions to make the call sites cleaner and to log errors.
Tried really hard not to mess with the cuddled braces ;)

…ded more methods for opening cameras. Wrapped xiParam functions for better error monitoring. Updated PLUGIN_LIB macros to work on ROS melodic. Removed region of interest check in favour of using the limits given by the camera. Broke openCam function down into smaller setFeature functions.

carloswanguw · 2020-02-06T16:10:54Z

I will review this tonight or within this week, thanks

…so reduced it to 2.0

shaun-createc · 2020-02-07T12:57:37Z

Hi Carlos(?), I'm hoping to use this driver in some up and coming projects. I like that is a nodelet and it looks like you have spent some time getting the settings right. I hope you don't mind the refactoring. With adding more checks I've introduced a few possible bugs. For cameras that do not support XI_PRM_ACQ_TIMING_MODE and XI_PRM_LIMIT_BANDWIDTH_MODE like the MQ the driver will now show an error when limiting frame rate and a warn when limiting bandwidth. In both cases I think this is a false negative. When I get the chance next week I'll alter it to allow these calls to fail without reporting a big red error.

…

________________________________ From: carloswanguw <[email protected]> Sent: 06 February 2020 16:10 To: wavelab/ximea_ros_cam <[email protected]> Cc: Shaun Prickett <[email protected]>; Author <[email protected]> Subject: Re: [wavelab/ximea_ros_cam] Updates (#43) I will review this tonight or within this week, thanks — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#43?email_source=notifications&email_token=AEBXA35S7MZUDCRXDCNIF5DRBQZA7A5CNFSM4KQ7PN62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEK7ZBIA#issuecomment-582979744>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AEBXA33ONN6DEJQMGRHYLZTRBQZA7ANCNFSM4KQ7PN6Q>. Shaun Prickett +44 (0) 1900 828112 [Createc]<http://createc.co.uk> Unit 4, Derwent Mills Commercial Park Cockermouth, Cumbria, CA130HT [createc] The content of this message is confidential. If you have received it by mistake, please inform us by an email reply and then delete the message. It is forbidden to copy, forward, or in any way reveal the contents of this message to anyone. The integrity and security of this email cannot be guaranteed over the Internet. Therefore, the sender will not be held liable for any damage caused by the message. Registered Name: Create Technologies Limited, Registered office: Unit 4, Derwent Mills, Cockermouth, CA13 0HT, United Kingdom. Registered Number: 07293648. Registered in England and Wales. www.createc.co.uk.

carloswanguw

Overall looks good! Thanks for reorganizing the code, I am fine with any additional code refactoring that you might have - I generally try to follow the google style guide however this was a quick camera driver hack I did a while back. I will leave you to do the physical testing of the driver on your cameras, under the assumption that it will still work on the original camera.

The only major comment I have is with regards to the timestamp estimation (see comments below). Lets also introduce a flag to enable/disable the usage of the camera timestamps in the scenario where the camera supports timestamps.

carloswanguw · 2020-02-09T03:49:44Z

src/ximea_ros_cam.cpp

+    }
+    int roundDown(int value, int increment)
+    {
+        return (value / increment) * increment;


Maybe a bit overkill, however I like to be more explicit in this line:

return (int) (value / increment) * increment

However this line is sufficient as it is

Are you ok with... return int(value / increment) * increment; ...to keep it c++ style?
I take it this is to reassure a reader who might assume int / int is not an integral division.

Are you ok with... return int(value / increment) * increment; ...to keep it c++ style?

I am fine with it - the original line is fine as well if that suits you better.

I take it this is to reassure a reader who might assume int / int is not an integral division.

The explicit int is meant to both show the reader and compiler no ambiguity in what the divide is meant to do. I am, of course being overly cautious here.

Speaking of being overly cautious, perhaps there should be a quick check here to avoid a divide by zero error =P

There should really be that div 0 check, something like...
return increment? int(value / increment) * increment : value;
or perhaps...
return increment? int(value / increment) * increment : 0;

carloswanguw · 2020-02-09T04:06:30Z

README.md

@@ -97,7 +100,11 @@ Copy the `example_cam.launch` and input another serial number to `serial_no` of

 ### General

-`serial_no` - Serial number of the Ximea camera (used to locate the proper camera)
+`serial_no` - Serial number of the Ximea camera (used to locate the correct camera)


Just a quick comment here, perhaps a better comment would be to state the order of precedence during the camera device open phase, in addition to each variable's definitions

serial_no

user_id

First camera on the bus

carloswanguw · 2020-02-09T04:08:30Z

src/ximea_ros_cam.cpp

+    }
+    else
+    {
+        ROS_INFO_STREAM("No serial number or user id provided. Opening first camera on bus");


period at end?

carloswanguw · 2020-02-09T04:10:42Z

src/ximea_ros_cam.cpp

-    // To avoid warnings
-    (void)xi_stat;
+        XimeaROSCam::openCam();
+    }   


Extra whitespaces

Yes sorry about that, I blame VSCode.
Purging...

carloswanguw · 2020-02-09T04:10:48Z

src/ximea_ros_cam.cpp

@@ -678,11 +578,9 @@ void XimeaROSCam::frameCaptureCb() {
        // Acquire image
        xi_stat = xiGetImage(this->xi_h_,
                             this->cam_img_cap_timeout_,
-                             &xi_img);
+                             &xi_img);         


Extra whitespaces

carloswanguw · 2020-02-09T13:50:44Z

src/ximea_ros_cam.cpp

+        // untested experimental handling!
+        ros::Duration wrap_period;
+        ros::Duration rough_wrap_period = 
+                (queue.front().second - queue.back().second) + 


Maybe a better way to estimate this period is to use:

(last timestamp > the second last timestamp for the camera) to compute the rough wrap period for the camera?

Might also be able to use XI_PRM_DEVICE_NAME to hardcode the wrap period

I think its safe to use as is but I will review some more, in practice it is rare for the deque to get longer than 3.

Is there a scenario where deque becomes longer than 3?

Yes it is possible, it depends on the lag between image capture and retrieval via xiGetImage together with frame rate. All the same I would not expect it to get very big.

I guess it would be prudent to add a size check in the sample function that displays a warn when the deque goes over say 6, perhaps even pop the front if it blows up due to some unknown discrepancy between the XI_IMAGE stamp and the retrieved stamp.

carloswanguw · 2020-02-09T13:51:54Z

src/ximea_ros_cam.cpp

+        const ros::Duration forty_bit_period(forty_bits / million, forty_bits % million);
+        const ros::Duration ros_duration_period(1<<31,0); // ros duration overflow
+
+        if((rough_wrap_period - forty_bit_period).sec == 0) 


Does this assume that the camera rate is > 1Hz?

No (I hope) because we account for the system time difference so I believe the error will be the drift between the camera and system clocks together with the round trip error which I imagine to be in the order of milliseconds. It really needs testing in some way though. I'm thinking of artificially simulating it rather than waiting for 12 days.

I am imagining a scenario where we capture at... lets say 0.5Hz, or 2 seconds per capture.
Each poll we wait for at least 2 seconds before the image arrives, meaning we have a span of greater than 2 seconds between timestamps.

rough_wrap_period will then have a difference of an order of magnitude of seconds (unless I am making the wrong assumption here), which makes the estimate rough_wrap_period - forty_bit_period).sec (which I think is the time difference between our estimated amount with the theoretical max of the camera, all converted to seconds) to have a large enough difference that it might not be 0, be perhaps -1 or -2 seconds depending on the rate.

Does this make sense? I might not be understanding the logic here well enough.

For example, let's exaggerate the situation and say we wanted to capture an image every 5 seconds (for timelapse).
And let's say we had a max wrap period of 11 seconds.

Then wouldn't our time steps in sequence be something like:

0, 5, 10, 4, 9, 3, ..., and if we assume that our deque ~~rotary buffer (wrong choice of words here)~~ is: 10, 4 which then we estimate the rough_wrap_period to be 10 - 4 = 6, we check this with the above line to be (6 - 11).sec which is 5 seconds, which this statement becomes false when it should be true?

carloswanguw · 2020-02-09T13:53:03Z

src/ximea_ros_cam.cpp

+        if((rough_wrap_period - forty_bit_period).sec == 0) 
+            wrap_period = forty_bit_period; // ~305 hours
+        else if((rough_wrap_period - ros_duration_period).sec == 0)  
+            wrap_period = ros::Duration(1<<31,0); // ~68 years (worlds longest running camera)


wrap_period = ros_duration_period;?

ros::Duration is signed where tsSec is unsigned and fully saturated for the the long period clock versions so I'm assuming its the ros::Duration that overflows first. I'm not confident I have this right.

Sorry I saw this

const ros::Duration ros_duration_period(1<<31,0);

And then I saw this:

wrap_period = ros::Duration(1<<31,0);

So I thought since they were both of type ros::Duration we can simply assign the constant to the wrap_period:

wrap_period = ros_duration_period;

However if there is a belief that the constructor of ros:Duration becomes strange when the overflow limit is passed into it, then it is best to leave this line as it is.

Ahh, yes I should of reused the constant. However it turns out ros::Duration is not suitable as it throws when we need it to naturally overflow. Thankfully you have no objections to std::chrono

carloswanguw · 2020-02-09T14:16:25Z

src/ximea_ros_cam.cpp

+        ROS_WARN_STREAM("Adjusting timestamps using a period of " << wrap_period);
+        using iterator = std::deque< std::pair<ros::Time,ros::Duration> >::iterator;
+        for(iterator it = queue.begin(); it != queue.end(); ++it)
+            if(it->second > queue.back().second)


~~It is~~ Is it an expected behaviour here, once queue.back().second reaches the wraparound (which means that queue.back().second reaches ~0), then we will subtract almost all of the previous values by the wrap around?

For example:
If we have a camera timer which wraps at 1.0, and we obtain timestamps every 0.2, with the first timestamp obtained at 0.1, then our first 6 timestamps will be:

0.1, 0.3, 0.5, 0.7, 0.9, 0.1

With this check, it means our result should be (with the it->second -= wrap_period statement):

0.1, -0.7, -0.5, -0.3, 0.1

which puts most of the timestamps in the negatives, as well as an edge case where the first (front) timestamp being still in the positive.

It is expected the the camera clock will not overflow twice during the period covered by the deque.
It is expected that when the wrap gets detected only the last pair in the deque will contain a wrapped stamp, this is because other than initialising we only sample the camera clock in the interpolation function where we correct previous stamps to be in the new time frame. The stamp from XI_IMAGE may or may not be wrapped and I realise there is a bug here where we correct the stamps in one frame but might still get images in with non wrapped stamps in subsequent frames. I guess another check could catch this.

I see what you mean here, I think I was reading through this in sequence and did not take into account of

while(queue.size() > 2 && queue[1].second < camera_stamp) { queue.pop_front(); }

The only scenario where I feel we will get a growing queue size is when the camera capture continuously times out and somehow the camera_stamp does not get updated.

Assuming that this is not the case and we will know when the camera is not behaving properly if there are timeouts, then you are right that the camera clock will not overflow twice.

I think just by the fact that the timestamps will never surpass the max, we will be working with negative timestamps in queue[0].second and camera_stamp for our interpolation since we are subtracting these with wrap_period.

This is fine though since the interpolation scalar computation is a proportional computation - so we should be good in the expected case.

As for the edge case where the image timestamp is not corrected (maybe this will only happen at a high frequency asynchronous scenario), good catch!

carloswanguw · 2020-02-09T14:18:44Z

src/ximea_ros_cam.cpp

+        for(iterator it = queue.begin(); it != queue.end(); ++it)
+            if(it->second > queue.back().second)
+                it->second -= wrap_period;
+        if(camera_stamp > queue.back().second)


Same as the comment above

carloswanguw · 2020-02-09T14:38:53Z

src/ximea_ros_cam.cpp

+    double scalar = (camera_stamp - queue[0].second).toSec() /
+            (queue[1].second - queue[0].second).toSec();
+    ros::Time system_stamp = queue[0].first + 
+            (queue[1].first - queue[1].first) * scalar;


queue[1].first - queue[0].first?

That is a bug. I need to test.

Oh no, I understand the math here and I believe that there was a typo, as

queue[1].first - queue[1].first pretty much is 0, so we are always going to assign the timestamp to be queue[0].first regardless on the scalar value.

carloswanguw · 2020-02-09T14:53:22Z

An additional comment will be to do some timing tests (start the camera at a low FPS, then a high FPS) and vary the CPU load in the computer to emulate variance in the timestamp acquisition period. Plot the camera timestamps + ROS timestamps and see if the estimation makes sense or if there are any edge cases we might not have accounted for (aside from the wraparound)

…idth to reported limits before trying to set. Fixed a few typos. Supressed warnings to get/set calls we know might fail

shaun-createc · 2020-02-10T17:06:41Z

Do you have any objections to bringing in some C++11 features? I'm finding the limits of the ros Time types frustrating, and I would like to use chrono.

carloswanguw · 2020-02-11T04:26:19Z

Do you have any objections to bringing in some C++11 features? I'm finding the limits of the ros Time types frustrating, and I would like to use chrono.

Feel free to add chrono, I don't have any objections to any standard lib that comes with C++11

shaun-createc · 2020-02-11T11:24:28Z

Thanks for all the useful comments.
I need to jump onto something else for a few days.
I dealt with a bug yesterday with the frame limit handling on certain models and I added the ability to suppress the warn to the get set functions. This for when we know the call might fail.
I had a chance to simulate a 33 bit nanosecond stamp where I found the ros::Time types throw, the overflow handling needs more work, which I'll try to fit in this week (camera is on loan)

carloswanguw · 2020-02-13T04:50:38Z

src/ximea_ros_cam.cpp

+
+    // interpolate stamp
+    double scalar = (camera_stamp - queue[0].second).toSec() /
+            (queue[1].second - queue[0].second).toSec();


Just realized we might also want to do a div by 0 check here as well

carloswanguw · 2020-02-13T04:52:10Z

Thanks for all the useful comments.
I need to jump onto something else for a few days.
I dealt with a bug yesterday with the frame limit handling on certain models and I added the ability to suppress the warn to the get set functions. This for when we know the call might fail.
I had a chance to simulate a 33 bit nanosecond stamp where I found the ros::Time types throw, the overflow handling needs more work, which I'll try to fit in this week (camera is on loan)

No problem, just continue with the updates and I will continue to review when I have the time

shaun-createc added 2 commits February 6, 2020 14:24

Updated README

0414212

Adjusted manual_gain value in example config to be floating point, al…

c948c1b

…so reduced it to 2.0

carloswanguw reviewed Feb 9, 2020

View reviewed changes

Fixed frame rate limit mode for CB, MC, MT, MX models. Clamping bandw…

ed4c3ce

…idth to reported limits before trying to set. Fixed a few typos. Supressed warnings to get/set calls we know might fail

carloswanguw reviewed Feb 13, 2020

View reviewed changes

Made Nodelet library explicitly SHARED

3c36aa5

Updates #43

Are you sure you want to change the base?

Updates #43

Conversation

shaun-createc commented Feb 6, 2020

carloswanguw commented Feb 6, 2020

shaun-createc commented Feb 7, 2020 via email

carloswanguw left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carloswanguw Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carloswanguw Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carloswanguw Feb 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carloswanguw commented Feb 9, 2020

shaun-createc commented Feb 10, 2020

carloswanguw commented Feb 11, 2020

shaun-createc commented Feb 11, 2020

Choose a reason for hiding this comment

carloswanguw commented Feb 13, 2020

carloswanguw left a comment •

edited

Loading

carloswanguw Feb 11, 2020 •

edited

Loading

carloswanguw Feb 11, 2020 •

edited

Loading

carloswanguw Feb 9, 2020 •

edited

Loading