search

anqixu / ueye_cam

A ROS nodelet and node that wraps the driver API for UEye cameras by IDS Imaging Development Systems GMBH.
Other
60 stars 102 forks source link

Fix IS_INVALID_BUFFER_SIZE bug. Consider absolute X and Y for memory … #107

Closed singhjeet1989 closed 3 years ago

singhjeet1989 commented 3 years ago

Issue:- [ERROR] [1611661455.769880480]: Could not start free-run live video mode for [camera] (IS_INVALID_BUFFER_SIZE) This error appears only when x and y coordinates are more than 0.

Solution:- Consider absolute value of X and Y for memory allocation when it's more than 0.

singhjeet1989 commented 3 years ago

Hey guys, Any update on this pull request?

nullket commented 3 years ago

Hey guys, Any update on this pull request?

I don't think so. As mentioned in my comment above it should work just fine as it is, as the allocated target memory is just the size of the resulting image. According to IDS the memory should be large enough and for that reasons the memory gets always allocated for the resulting image: The current master takes cam_aoi_.s32Width (same for height) which is set by setResolution() which is called in the nodelet (reconfigure, launch parameter and parameters saved on the cam).

We could of course allocate memory for the whole sensor and fill it just with the data for width and height, but this seems like a waste of resources and might induce more problems with the ros message (as seen in you PR, the need of shifting the cam_buffer_ptr)

Instead of having always a large memory buffer it would be nice to figure out why it does not work as it should and fix this instead?

nullket commented 3 years ago

Maybe the .ini routine is causing this bug, you should test using a launch file setting aoi Change some settings and try this snippet:

Click to see launch file with aoi

``` ```

singhjeet1989 commented 3 years ago

@nullket Well this issues can't be reproduced with launch file because the parameter which is causing the problem isn't supported by ROS launch file (http://wiki.ros.org/ueye_cam). This is reproducible only when Start X or Y is non zero and Start X absolute or Y absolute is set to 1. image Apparently I couldn't find any other root cause for this issue, let me know if you have better suggestion or solution.

singhjeet1989 commented 3 years ago

Implementation has been improved, please have look at new commit.

nullket commented 3 years ago

and Start X absolute or Y absolute is set to 1.

Mhm, I think I slowly get your problem. So the issue is that you use the parameters Start x/y absolute = 1 right? It seems correct that this parameter can not be set via launch file.

So when you use this parameter with a value of 1 the buffer will be to small as in the current implementation the buffer is always only as big as the the aoi. But this parameter forces to place the image in the middle (or wherever your aoi is) of the buffer. Then the buffer would need to be larger than just the AOI. This would only work (or make sense) when the buffer would be as big as sensor.

But maybe you can explain why you would want to use this parameter anyways? The current implementation supports AOI (in an efficient way of making the buffer not to large) right? So why would you want to force the driver to use a larger buffer and then throw parts of it away when publishing a ros topic? Maybe I still do not get the whole picture or idea behind this, in case please explain the problem a bit more.

Are you forced to use those ini files without beeing able to change Start x/y absolute to 0? Another solution would be to set this parameter to 0 during syncCamConfig and the ini File parsing.

(The AOI start x/y as well as width/height can be set via launch file).

singhjeet1989 commented 3 years ago

Indeed as I mentioned problem occurs only when Start X absolute or Y absolute is set to 1.

Using ini file is my customer requirement, customer wants to play with camera parameters in uEye Cockpit and generate a ini file. Apparently the customer expects to work cockpit generated ini with ROS driver. It is not under the controller of SW developer to tune ini file. However it is possible to tune parameters in ROS driver in syncCamConfig function like you mentioned. I will try this approach and come back with outcome.

singhjeet1989 commented 3 years ago

@nullket Well I quickly looked at uEye interface (uEye.h) for setting Start X absolute or Y absolute to 0. It turns out that uEye driver doesn't offer interface for setting. image So I don't think there is other possible solution for this issue, let me know if you have any other suggestion.

nullket commented 3 years ago

Yes, and I did not expect the IDS Driver to have such a setting. What I meant is the handling withinueye_cam_driver.cpp of this package (both things are called driver around here...).

The AOI might have stored this "absolute thing" at two points:

The latter of those two calls the first and has no good possibility of interference (except of writing an overly-complicated ini parser..). Therefore, one could handle the existence check of IS_AOI_IMAGE_POS_ABSOLUTE and if it exists, give a warning and set the AOI without the IS_AOI_IMAGE_POS_ABSOLUTE (which simply adds 0x10000000 to the AOI) in syncCamConfig with setResolution to the camera.

To be really honest, this solution would not introduce new problems in the whole buffer handling (which is called, recalled, adjusted several times within this package. Therefore, I like the idea of ignoring the "absolute" setting like explained above. It brings no advantage for this ros package (of course dealing with should be supported). Nonetheless, my above proposed way is also somewhat nasty in terms of cleanness: We would set some values during syncCamConfig. @jmackay2 do you have an opinion on this?

jmackay2 commented 3 years ago

@nullket That solution seems fine. It does make the AOI stuff a little messier, but I think it is the most straight-forward approach at the moment.

singhjeet1989 commented 3 years ago

Dear all reviewer, I would really appreciate if we can conclude this pull request.

nullket commented 3 years ago

Dear all reviewer, I would really appreciate if we can conclude this pull request.

Dear @singhjeet1989 I totally understand your wish to get this merged! I do not like open PRs either ;)

But as I mentioned in https://github.com/anqixu/ueye_cam/pull/107#issuecomment-823009417 and @jmackay2 confirmed, we think it would be better to not touch the memory handling. I would prefer your way on a plain new driver but here it could have severe side effects (the memory handling is quite a bit confusing as it happens in multiple places. For that reason I suggested (and pointed out the places in the code) where we could simply check for and then ignore IS_AOI_IMAGE_POS_ABSOLUTE is that not something that would work for you and would let me sleep better as we do not touch to much here?

If yes, please change your PR and provide the mentioned "check and ignore" code. I am happy to merge and help you with this.

singhjeet1989 commented 3 years ago

@nullket I understand your concern w.r.t memory handling so let's not touch memory part in this package. Coming back to your proposed solution, I didn't understand how to ignore IS_AOI_IMAGE_POS_ABSOLUTE? Do you mean reset cam_aoi_.s32X and cam_aoi_.s32Y to 0 in syncCamConfig function if IS_AOI_IMAGE_POS_ABSOLUTE exist? UINT start_posX_absolute = 0; UINT start_posY_absolute = 0;

if ((is_err = is_AOI(cam_handle_, IS_AOI_IMAGE_GET_POS_X_ABS, (void*) &start_posX_absolute, sizeof(start_posX_absolute))) != IS_SUCCESS) { ERROR_STREAM("Could not retrieve Area Of Interest (AOI) information for parameter start X absolute for[" << cam_name_ << "] (" << err2str(is_err) << ")"); return is_err; }

if (start_posX_absolute == IS_AOI_IMAGE_POS_ABSOLUTE) { cam_aoi_.s32X = 0; }

if ((is_err = is_AOI(cam_handle_, IS_AOI_IMAGE_GET_POS_Y_ABS, (void*) &start_posY_absolute, sizeof(start_posY_absolute))) != IS_SUCCESS) { ERROR_STREAM("Could not retrieve Area Of Interest (AOI) information for parameter start Y absolute for[" << cam_name_ << "] (" << err2str(is_err) << ")"); return is_err; }

if (start_posY_absolute == IS_AOI_IMAGE_POS_ABSOLUTE) { cam_aoi_.s32Y = 0; }

nullket commented 3 years ago

Do you mean reset cam_aoi_.s32X and cam_aoi_.s32Y to 0 in syncCamConfig function if IS_AOI_IMAGE_POS_ABSOLUTE exist?

Sorry for the late response, it was a busy week. Yes this is exactly what I meant.

I am looking forward to your commit on this branch!

singhjeet1989 commented 3 years ago

@nullket Thank you for feedback. Changes has been committed, please review.

nullket commented 3 years ago

@nullket Thank you for feedback. Changes has been committed, please review.

I did some init testing and so far it works. I will review it further as soon as I find some time (hopefully this week). Can you upload (as a comment) an ini file which definitely failed before?

singhjeet1989 commented 3 years ago

@nullket Thank you feedback. Please find attached config file. invalid_buffer_size_config.txt

nullket commented 3 years ago

@singhjeet1989

Please checkout this branch https://github.com/anqixu/ueye_cam/commit/dca6a07747e0aaea7ee2df06e5f9dc09a851e08f#diff-25a6634263c1b1f6fc4697a04e2b9904ea4b042a89af59dc93ec1f5d44848a26

It works great for me. Please add the changes to your PR in case you agree.

Btw the ini file provided by you works on my cam (UI527xCP) on master without any issue.

singhjeet1989 commented 3 years ago

Btw the ini file provided by you works on my cam (UI527xCP) on master without any issue.

Good to know that same ini works with different sensor. Unfortunately it doesn't work with "UI320xSE-M". Apparently it is an indication that some sensor is expecting memory allocation for entire AOI when absolute position is set.

I do agree with your suggestion, and changes has been made accordingly.