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

Features added & major rewrite. #42

Open
wants to merge 60 commits into
base: master
Choose a base branch
from

Conversation

peci1
Copy link

@peci1 peci1 commented Oct 11, 2015

Hi guys, in our TRADR project we use an Axis 214 PTZ cam on a mobile robotic platform running ROS.

So we started using this driver, but as time went, we needed more features than it currently provides.

We were thinking about the best way how to add the features we need, and finally we agreed that the current API has some flaws (e.g. mixing focus/brightness control together with pan/tilt/zoom) we would also like to fix.

So we started a complete rework of the driver, and we achieved the following as a result:

  • The driver is ready to be used with more Axis camera types than just the 214 PTZ (because we implemented a large part of the VAPIX API communication).
  • There is a camera model displayable in RViz, the camera reports its pan/tilt/zoom as both a PTZ message, JointStates message, and via TF
  • More parameters controllable via dynamic_reconfigure: video stream parameters (resolution, FPS etc.)
  • Code structure changes for "better" (in our view, of course) support of current Python and OOP recommendations.
  • Renamed methods and members according to PEP 8 Python naming standard (if it's there, why not use it)
  • We tried as hard as we could to retain backwards compatibility with the previous version (by adding extra backwards compatibility layers translating old code usage into the new one). We haven't renamed any topics from the current version. The camera stream is published on the same topic in the new code, and the PTZ-related topics are still there, just deprecated.

Please, take a look at the new code (looking at the changelog is probably not a good idea since we changed too much). Tell us if you like the changes we made, and, please, test it with your existing applications utilizing the old API to be sure the backwards-compatibility layer works as expected.

Also, please, tell us if the backwards compatibility layer is needed at all. If it weren't, some more structural changes towards clean design could be done.

Thank you very much and hope you'll like it.

peci1 and others added 30 commits August 31, 2015 17:24
 Refactored axis.py to use OOP in a more suitable way.
 Added the possibility to specify compression and FPS levels.
…nes are retained for BC.

Also changed the resolution select to use the standard CIF resolution names.
Also, when the driver cannot connect to VAPIX, it now tries forever to connect again.
I tried to make the parameters reflect the real state of things, but it seems it ïs a bit buggy - the most if one uses the deprecated PTZ config together with Camera.
…led TF publishing.

 Added a simple way how to display both camera image and camera pose in RViz.
Not very much configurable as of now, but it works.
…ed (requires anonymous PTZ control, or providing credentials).
…- the hostname was therefore prepended twice.
It still needs more refactoring so that it doesn't use the self._axis variable.
It still needs more refactoring so that it doesn't use the self._axis variable.
@awesomebytes
Copy link
Member

Hey I tried, If I send a 0.0 I get:

Calling PTZ command continuouszoommove=0 on host 10.68.0.6, camera 1

Opening VAPIX URL http://10.68.0.6/axis-cgi/com/ptz.cgi?camera=1&continuouszoommove=0 .

If I send 0.00001:

Calling PTZ command continuouspantiltmove=0,0&continuouszoommove=0 on host 10.68.0.6, camera 1

Opening VAPIX URL http://10.68.0.6/axis-cgi/com/ptz.cgi?camera=1&continuouspantiltmove=0,0&continuouszoommove=0 .

Seems like the 0.0 is treated as a "not filled field" value, or something.

@peci1
Copy link
Author

peci1 commented Oct 30, 2015

@awesomebytes Thanks for the info, I hope the bug is fixed now. Please, try to update your branch and test it again.

@jeff-o
Copy link
Contributor

jeff-o commented Oct 30, 2015

I've been watching these changes with interest since I use Axis PTZ cameras
quite often. I tried using the driver with an Axis P5512-E, and
unfortunately it did not work. If the plan is to merge these changes in
with the main branch then it will either need to be differentiated as being
exclusively for the Axis 214, or P5512 support will need to be built in.

In a week or so I'll be receiving a new Axis 214 camera to try out with
this driver, I'll let you know how it goes.

On Thu, Oct 29, 2015 at 9:00 PM, Martin Pecka [email protected]
wrote:

@awesomebytes https://github.com/awesomebytes Thanks for the info, I
hope the bug is fixed now. Please, try to update your branch and test it
again.


Reply to this email directly or view it on GitHub
#42 (comment)
.

Jeff Schmidt
System Integration Technologist
Clearpath Robotics
Phone: 519-513-2416

@peci1
Copy link
Author

peci1 commented Nov 2, 2015

Jeff, according to the documentation, your P5512-E camera should also support the VAPIX API and therefore should be supported by the driver.

I think we can debug the cause. I just don't think we need to do the communication here. I left you an email message and let's continue there ;)

peci1 and others added 5 commits November 2, 2015 01:25
Now the proposed changes to ROS wiki can be saved and discussed somewhere until they are published.

To "compile" the wiki page, just open any ROS wiki page, click Edit (Text), paste the contents there and click Preview.
…am.cfg.

This uses kind of a hack from dynamic_reconfigure_tools, but it works well.
@peci1
Copy link
Author

peci1 commented Nov 5, 2015

I did an architectural change to the way how resolutions are handled. Because there are going to be numerous resolutions for different Axis products, it seemed to me impractical to create a list of supported resolutions, from which only a few would be supported by particular camera models.

So in the last 2 commits I did a little hack that exploits the dynamic_reconfigure API and dynamically generates the list of supported resolutions based on what the camera reports. It is non-standard code, but I tested it causes no problems when using either dynparam, rqt_reconfigure or the Python client.

I also improved the error messages when connecting to the API doesn't succeed. Now, it the camera returns 401 Unauthorized, the log message instructs the developer to provide a username/password or to allow anonymous access.

peci1 and others added 4 commits November 6, 2015 03:06
…upports them.

Otherwise, it prevented the driver to run on e.g. cameras with no absolute iris control (just autoiris).
Added the option to choose a resolution by specifying only one dimension.
peci1 and others added 6 commits February 24, 2016 16:04
This may happen when you loop a bag file and stream the live camera at the same time.
This is the easiest fix for selecting our package over the ROS-bundled
one. This might be a bit hacky, but bumping the version does make sense
anyway.
@cclauss
Copy link
Contributor

cclauss commented Jul 14, 2018

Status?

@peci1
Copy link
Author

peci1 commented Jul 16, 2018

Conflicts of this branch with master should be easy to resolve. However, I'm still not sure the maintainers (if there are still any) are interested in this different approach. Maybe forking and creating an alternative driver would be better?

@civerachb-cpr
Copy link
Collaborator

Hi, this package was orphaned for a long time. Clearpath has just (re-)assumed maintainership. We're looking into this now and will likely be merging it soon.

@peci1
Copy link
Author

peci1 commented May 12, 2020

Wow. Haven't expected anyone will ever resurrect this :) I'm no longer actively using the camera, so I can't tell whether the code still works or if it is compatible with newer models.

@civerachb-cpr
Copy link
Collaborator

We'll make sure things are still working before merging things into master. But given how long this project has been dormant, any improvements are welcome.
For now I've merged the changes into a new branch (pr-42) so we can test things out more. Assuming that goes well we should be able to finally close out this PR.

@civerachb-cpr
Copy link
Collaborator

I finally had a chance to try out your changes with an Axis M5525
Unfortunately it appears there are some bugs that are preventing the driver from properly detecting the camera settings: launching the node gives the following errors & warnings:

[WARN] [1590506707.956746]: Could not determine resolutions supported by the camera. Asssuming only CIF. [ERROR] [1590506740.601021]: HTTPError() [ERROR] [1590506741.606222]: HTTPError() [ERROR] [1590506742.612564]: HTTPError() [ERROR] [1590506743.618947]: HTTPError() [...]

The HTTPError messages only start appearing once I add the image topic in rviz.
The current master & 0.3.0 versions do work correctly with this camera.

@peci1
Copy link
Author

peci1 commented May 26, 2020

Thanks for testing. Could you try to get some deeper debug info? What are the HTTP errors about? Can you try to change the source code so that the content of the error is displayed?

@civerachb-cpr
Copy link
Collaborator

I'll do my best, but I don't have access to a camera right now; I did as much testing as I could before one of our integrators took the camera away to install it on a customer's robot. I'm working on getting longer-term access to a camera to do some more thorough work on this driver soon though.

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.

6 participants