Spinnaker Driver Rework by guyfleeman · Pull Request #45 · RoboCup-SSL/ssl-vision-processor

guyfleeman · 2026-10-21T02:48:00Z

This is the initial attempt at a spinnaker driver rework, properly supporting mix-generation spinnaker cameras and covering configuration edge cases that were not supported. These changes were tested on two BFLY-PGE-13E4C cameras, and still need testing on league cameras.

This PR incorporates #42 and #44.

Let me know you want this split in to multiple PRs.

Summary:

~150 LoC nix env/driver readme
~400 LoC core driver updates. Mostly auto bayer mode, exception handling, startup nvm race condition logging
~600 LoC spinnaker_probe utility which very pedantically enumerates camera features in the API. Was very useful for understanding differences in access functions offered for the league cameras and our PoE cameras.

Items discovered and fixed:

The reset camera functionality only fully occurs after a reboot, despite seeming to indicate otherwise.
When invoking the persistent storage reset, the call can sometimes fail due a race condition on startup or if the API previously faulted and reconnected. In these instances the camera takes extra time to zero for some reason, but the API doesn't report it. Waiting and reissuing the command always fixes it. This occurs seeming <1-5% of the time. A fair amount of this rework logs this behavior to understand it better (hopefully at scale).
Bayer modes are no longer hard coded. The driver queries the camera for supported bayer modes and attempts to find one the CL kernels support. This now prints clearly if no match can be found.
Different generations of camera use different API function calls for the same function, this is now abstracted so features like "exposure" or "gamma" are set across generations.
Some parameters were set in modes where setting was silently ignore or outright invalid. This included resolution and gamma.
The league typically uses NewestOnly for the packet acquisition, but supported other modes historically, some of which faulted because buffer depth was too shallow. This is fixed.
PoE/GigE camera ID are technically unstable across reboots according to the Spinnaker API. I've never seen them not enumerate the same way, but they cannot be relied upon to index cameras. This adds serial flag to bind instances. Get a serial with "build/spinnaker_probe --list" or use spinview instead of cam_id.
AcquisitionFrameRate is wildly inconsistent across models, with no fewer than 4 ways to access this value which may or may not be present depending on the model. The driver now tries to find any valid candidate, in order of newest to oldest.
All accesses are now guarded with readable/writable checks, and top level function bodies log spurious spinnaker exceptions before segfaulting. Overall debug clarity is substantially improved.

New features:

Nix flake support for Nvidia CL drivers
spinnaker_probe binary to dump full low-level camera API functions as json

…-processor into dev/will/spinnaker_driver_rework

lixfel

I'm not yet completely through, but these are my first thoughts on the pull request.

I would also prefer if this PR would be based and intended to be merged to master, as this should be independent of the new UI interface.

lixfel · 2026-10-21T04:11:54Z

+- Renamed: `GammaEnabled` instead of `GammaEnable`.
+- Missing: `AcquisitionResultingFrameRate`, `AutoExposureMeteringMode`, `AutoExposureControlPriority`,
+  `BalanceWhiteAutoProfile`.
+- Gamma is unavailable in Raw/Bayer pixel formats.


This is definitely not the case on the league camera.

Okay. I think in this case the code does work but the docs are wrong. I'll confirm and address both as needed.

lixfel · 2026-10-21T04:14:58Z

  # Spinnaker, mvIMPACT or OpenCV(v4l2) camera id
  #id: 0
+  # Spinnaker camera serial number (if set id is ignored). Recommended with multiple cameras, as id order is not stable.
+  #serial: "12345678"


As I want to keep the API (especially with the new network interface) uniform across drivers: How about sorting cameras by serial id? Then you have a guaranteed linear numerical id (even though the id order has never been observed as unstable).

Because otherwise we would need some tooling advertising available cameras.

A few potential gotchas here. If you are running multiple cameras and one drops and system restarts they'll all renumber sorting by serial ID, but I think that's trending in the right direction. This could occur with PoE power cycle, or USB DC. This only impacts setups where more than one camera is connect to a physical machine. e.g. not SSL comp, but does affect PTO, and at least two labs I know of (A-Team and RJ).

What if we sort by serial to assign ID as you suggest. OpenCV already takes a path, we could do something similar and change it to "device" or some other key, and then that would be uniform across drivers. For OpenCV that'd be a path (one of the by UUID or bus paths), on spinnaker or mvimpact a serial number. Future drivers should be able to offer one or the other.

The spinnaker_probe binary here does have basic enumeration capability. We could rip that subset out.

Yeah, or reuse the path, i think that's a good idea, maybe renaming it to "device", for OpenCV it's a path, for Spinnaker the serial id, for Mvimpact unused for now.

I will do this.

lixfel · 2026-10-21T04:16:16Z

+	configurePacketSize();
+}
+
+void SpinnakerDriver::reportOnce(const std::string& message) {


I think this feature makes a lot of sense in a generic context. Maybe move to log.h?

lixfel · 2026-10-21T04:22:36Z

 	Spinnaker::CameraPtr pCam;

+	// Acquisition starts on the first readImage(), so the settings applied after construction
+	// (resolution in particular) can still be written; the camera locks them while streaming.


As this will be unintuitive when doing reconfiguration via the web ui (and possibly impossible): When changing resolution, we should instead do in setResolution(): stop streaming, change resolution, restart streaming. Because we will hit readImage with 99% certainty before receiving our first config...

And then we don't need the acquiring stuff

lixfel · 2026-10-21T04:24:39Z

+
+	if(frametime == 0.0)
+		WARN("Camera reports no frame rate, frame time overruns will not be detected");
+	frametime = INFINITY;


I would set it to 30 FPS (like in OpenCV with files), as this I would consider a bottom line we should definitely warn about.

And maybe we should warnOnce about the no reported framerate.

lixfel · 2026-10-21T04:26:55Z

+	if(!acquiring)
+		startAcquisition();
+
+	// The resulting frame rate changes with (auto) exposure, so poll it occasionally


To detect a runaway exposure I would recommend against regularily polling the frame rate, maybe just once after setting the exposure? (would simplify things as well)

This should be conditional. With AutoExposure + frame-rate control off, this will drift. We can only poll when the combo exists to reduce overhead since the league unlikely to want AutoExposure + no frame rate control.

lixfel · 2026-10-21T04:27:56Z

-		// Spinnaker buffer sizes need to be 1024 byte aligned
-		std::shared_ptr<RawImage> buffer = std::make_shared<RawImage>(CLArray((width * height + 1023)/1024 * 1024), &PixelFormat::RGGB8, width/2, height/2, "spinnaker");
+	const int width = (int)pCam->Width.GetValue();
+	const int height = (int)pCam->Height.GetValue();


Is there an issue with just allocating max size buffers once?

Yes but RawImage is const w/h. We can push this down to the RawImgae wrapper layer and only allocate once at max size.

lixfel · 2026-10-21T04:29:34Z

 float SpinnakerDriver::getGamma() {
-    return pCam->Gamma.GetValue();
+	// First generation cameras apply no gamma to Raw/Bayer formats, the node is unavailable then
+	Spinnaker::GenApi::CFloatPtr gamma = pCam->GetNodeMap().GetNode("Gamma");


Is there a difference between pCam->Gamma and pCam->GetNodeMap().GetNode("Gamma")? If not I would prefer the former.

This can be done.

lixfel · 2026-10-21T04:32:04Z

+	return nullptr;
+}
+
+bool SpinnakerDriver::setBool(const SpinnakerFeature& feature, bool value) {


I'm not 100% familiar with C++ template shenanigans, but wouldn't it be possible to have a generic set-function which templates to the different types?

This can be done.

lixfel · 2026-10-21T04:34:41Z

+	}
+}
+
+bool SpinnakerDriver::setEnum(const SpinnakerFeature& feature, const char* entry) {


As the API Enums shouldn't change depending on camera, can't we use the actual enum constants and enum entries instead of strings?

This can mostly be done, but will have some mapping for Bayer matching still. I'll take a crack at cleaning it up.

barulicm · 2026-10-25T06:33:12Z

+		const std::string pattern = entry != "Raw8" ? entry.substr(0, entry.size() - 1)
+				: IsReadable(pCam->PixelColorFilter) ? pCam->PixelColorFilter.ToString().c_str() : "";


While a little more verbose, an immediately-invoked lambda may be more readable than nested conditional operators. This is just a preference thing.

Suggested change

const std::string pattern = entry != "Raw8" ? entry.substr(0, entry.size() - 1)

: IsReadable(pCam->PixelColorFilter) ? pCam->PixelColorFilter.ToString().c_str() : "";

const std::string pattern = []{

if (entry != "Raw8") {

return entry.substr(0, entry.size() - 1);

} else if (IsReadable(pCam->PixelColorFilter)) {

return pCam->PixelColorFilter.ToString().c_str();

} else {

return "";

}

}();

guyfleeman added 7 commits October 20, 2026 11:22

improve spinnaker linking behavior

fec927a

mlock inversion, stick bit, double free

55effea

Merge branch 'dev/will/cl_dfree' of github.com:RoboCup-SSL/ssl-vision…

3ac9d4f

…-processor into dev/will/spinnaker_driver_rework

initial behavioral bug fixes

dc4bf81

draft of spinnaker driver rework

9372de8

local driver working on GigE

6d76a88

debug/retry logic for default param set race condition

5358891

guyfleeman self-assigned this Oct 21, 2026

guyfleeman requested a review from lixfel October 21, 2026 02:48

lixfel reviewed Oct 21, 2026

View reviewed changes

implement review items, improve error messages, C++ best practices

8d7bd70

guyfleeman mentioned this pull request Oct 24, 2026

Spinnaker Driver Improvement #46

Open

barulicm approved these changes Oct 25, 2026

View reviewed changes

		const std::string pattern = entry != "Raw8" ? entry.substr(0, entry.size() - 1)
		: IsReadable(pCam->PixelColorFilter) ? pCam->PixelColorFilter.ToString().c_str() : "";

-		const std::string pattern = entry != "Raw8" ? entry.substr(0, entry.size() - 1)
-				: IsReadable(pCam->PixelColorFilter) ? pCam->PixelColorFilter.ToString().c_str() : "";
+		const std::string pattern = []{
+			if (entry != "Raw8") {
+				return entry.substr(0, entry.size() - 1);
+			} else if (IsReadable(pCam->PixelColorFilter)) {
+				return pCam->PixelColorFilter.ToString().c_str();
+			} else {
+				return "";
+			}
+		}();

Conversation

guyfleeman commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lixfel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guyfleeman Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lixfel Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

guyfleeman commented Oct 21, 2026 •

edited

Loading

guyfleeman Oct 21, 2026 •

edited

Loading

lixfel Oct 22, 2026 •

edited

Loading