Skip to content

sigmastar: propose factory IMX334 IQ separately for review - #6

Draft
longsslz wants to merge 1 commit into
OpenIPC:masterfrom
longsslz:add-imx334-factory-iq
Draft

longsslz wants to merge 1 commit into
OpenIPC:masterfrom
longsslz:add-imx334-factory-iq

Conversation

@longsslz

Copy link
Copy Markdown

Summary

Separate IQ proposal from driver PR #5, based directly on upstream master. Contains only imx334.bin and provenance/configuration notes.

Provenance

Extracted from the original firmware of the SSC338Q / IMX334 camera used in the port. Not independently authored. Public redistribution authorization has not been established; please review acceptability and distribution requirements before merging. No claim is made that the repository license relicenses this binary.

Size: 86,944 bytes. SHA-256: cb267960d60c9c646516255c5d6c80f8991ce8efd861ae7c69974a8c29f24e92. Matches the RC5 file and the checked camera file.

Loading check

Setting isp.sensorConfig to /etc/sensors/imx334.bin prevented automatic selection of imx335.bin after video-service restarts. All six linear mode transitions loaded it successfully. This is not exhaustive image-quality validation, and HDR suitability is unverified.

Submitted as a draft for separate review; no driver changes or credentials included.

@openipc-ai openipc-ai left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splitting this out of #5 was the right instinct, and the disclosure here is exactly what we'd want from anyone proposing a binary. But I'm requesting changes: I don't think this file can land in this repo, and I don't think the question is close.

This repo has never tracked a binary. Everything in it is source: 69 .c, 23 .h, the Makefiles, and five text files. An 87 KB opaque tuning blob would be a new category for the project, and there's no build step here that consumes it — the Makefile globs *.c.

It's also the wrong repo geographically. sigmastar/infinity6e/ is a kernel-module build directory. The file's actual destination is /etc/sensors/, which ships from the firmware side of OpenIPC, not from here. Even with clean redistribution rights this wouldn't be where it goes.

Rights are unestablished, by your own account. You say plainly that this is an extracted third-party profile, not independently authored, and that public redistribution authorization hasn't been established. That's the honest framing and I appreciate it — but with no rights established and no precedent for binaries here, the default answer has to be no. I'm not able to accept it on "please review acceptability"; that's a determination that would need to come the other way, from whoever holds the rights.

What I would like to keep is the documentation. The finding on lines 20-22 — that Majestic falls back to imx335.bin after a video-service restart unless isp.sensorConfig is set explicitly — is genuinely useful and is the actionable half of this PR. It belongs in the driver's own notes in #5, where someone hitting the same fallback will find it.

Suggested disposition: close this PR, move the loading note into #5's IMX334.md, and leave the IQ profile out of the tree. Nothing here reflects badly on the contribution — separating it out and labelling it this carefully was the correct thing to do.

@@ -0,0 +1,24 @@
# Factory IMX334 daytime IQ

imx334.bin was extracted from the original firmware of the SSC338Q / IMX334

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub won't let me anchor a review comment on imx334.bin itself, so putting it here on the line that describes it.

This is the file I can't take, for the reasons in the review body: no precedent for binaries in this repo, wrong repo for an /etc/sensors/ artifact, and redistribution rights that the PR itself says are unestablished.

To be explicit about one thing so it isn't ambiguous later: the repo's MIT LICENSE covers OpenIPC's own contributions and does not relicense a third-party binary dropped into the tree. Your PR text already says you're not claiming that — I'm just confirming that reading is correct, so nobody infers otherwise from the file's location.


imx334.bin was extracted from the original firmware of the SSC338Q / IMX334
camera used for the driver port. It is not an independently authored profile.
Public redistribution authorization has not been established. Maintainer review

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence is the blocker, and I want to credit it rather than bury it — you flagged the problem yourself instead of letting it through quietly, which is the right instinct.

The difficulty is that "authorization has not been established" can't be resolved by maintainer review. We can decide whether we want a file; we can't grant rights we don't hold. Establishing them would mean the rights holder saying so, and absent that the answer defaults to no rather than to "probably fine".

`cb267960d60c9c646516255c5d6c80f8991ce8efd861ae7c69974a8c29f24e92`.
The file matches the project's RC5 release and the camera's checked IQ file.

For a compatible firmware, install as /etc/sensors/imx334.bin and explicitly set:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is also the argument against the file's location. /etc/sensors/ is populated from the firmware side of OpenIPC; this directory is a kernel-module build tree whose Makefile globs *.c and would ignore the blob entirely.

So even in the world where redistribution were settled, the file wouldn't ship from here — it would need to go wherever the rest of /etc/sensors/ comes from.

sensorConfig: /etc/sensors/imx334.bin
```

This explicit setting avoided Majestic automatically choosing imx335.bin after

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the part worth saving. The imx335.bin fallback after a video-service restart is a real, reproducible integration gotcha, and someone bringing up an IMX334 will hit it.

Could you move these three lines (plus the isp.sensorConfig snippet above) into IMX334.md in #5? The driver PR already mentions the IQ reload inconsistency in its description but not in the tracked docs, and this is the concrete workaround for it.

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