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

Check if camera supports sensor scaling before trying to set it #89

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nullket
Copy link
Collaborator

@nullket nullket commented Oct 7, 2020

Not all cameras support sensor scaling. This results in warnings as the driver fails to set the sensor scaling. I have added a simple check that figures out if the camera supports sensor scaling and only if so tries to set the value according to the parameter.

@jmackay2
Copy link
Collaborator

I am a little hesitant on this one. If the camera doesn't support sensor scaling, it may be nice to have the error message that the sensor scaling wasn't able to be set. I think the error message could be helpful.

@nullket
Copy link
Collaborator Author

nullket commented Dec 23, 2020

True. No I do not like my solution anymore either. But I also want to solve the issue that I get "warnings" whenever I launch the driver with my camera - even the setting is not specified in the launch file.

The issue is that scaling is also a ros parameter and those are getting parsed (what is okay) and set for all parameters (not okay) in parseROSParams(), which is called during the init connection process with the camera. In parseROSParams() the functions to set the parameters on the hardware are called as soon as one parameter is new (the whole hasNewParams thing.). So setSensorScaling() in L560 will be called even if it has not changed or was reconfigured.

One could create a whole construct of "has this particular parameter changed?" . If yes call the function to transfer the value to the hardware. This would blew up the code even further. Maybe some refactoring would help a lot with this issue. But this will be quite a lot of work to get rid of a warning which is not always true. Do you have another idea?

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