Convert ADOdin to FastCS-Odin

[shihab-dls]

closes #551

This PR implement FastCS-Odin in place of ADOdin.

[coretl]

Can we make this odin rather than od?

[coretl]

@claude

[coretl]

@claude

[coretl]

@claude

[claude]

Claude finished @coretl's task in 1m 52s —— View job

---

Code Review Complete

Tasks

• Read repository guidelines
• Review new FastCS Odin IO implementation
• Review Eiger detector changes
• Review Jungfrau detector changes
• Review test updates
• Check if previous review comments addressed
• Provide comprehensive feedback

---

Summary

This PR successfully migrates from EPICS ADOdin to FastCS-Odin. Previous review comments have been addressed. The code is generally well-structured with good test coverage. I've identified a few issues:

Issues Found

Critical:

• OdinWriter.open() always sets fp.frames = 0, potentially losing intended frame count configuration

Major:

• Naming inconsistency: od used instead of odin in EigerDetector (already flagged by @coretl)
• Jungfrau has duplicate OdinHdfIO class that should be removed once FastCS updates

Minor:

• wait_for_value() call uses single-item tuple unnecessarily
• asyncio.gather() with single coroutine in multiple places

Detailed Feedback

src/ophyd_async/fastcs/odin/_odin_io.py:76

The fp.frames.set(0) call appears to discard the exposure information. This was changed from exposures_per_event in commit 5d61dec.

Question: Is this intentional? If fp.frames represents expected frame count, setting it to 0 seems incorrect. If it's meant to reset a counter, the parameter name is misleading.

src/ophyd_async/fastcs/odin/_odin_io.py:87-89

await asyncio.gather(
    wait_for_value(self._drv.fp.writing, True, timeout=DEFAULT_TIMEOUT),
)

The single-item gather() is unnecessary. Use: await wait_for_value(self._drv.fp.writing, True, timeout=DEFAULT_TIMEOUT)

src/ophyd_async/fastcs/eiger/_eiger.py:23

od: OdinHdfIO

Should be odin: OdinHdfIO for clarity (as noted by @coretl). The 2-letter abbreviation makes the code less readable.

src/ophyd_async/fastcs/jungfrau/_jungfrau.py:9-17

The duplicate OdinHdfIO class with custom __init__ is a temporary workaround. The TODO comment indicates this should be removed once FastCS Jungfrau provides top-level 'detector' and 'odin' similar to EigerDetector.

Line 35: Type ignore comment

self.odin,  # type: ignore

This type ignore is needed because of the duplicate OdinHdfIO class. Will be resolved when the TODO is addressed.

Positive Observations

• ✅ All previous review comments addressed (unused Reference import removed, wait=True defaults removed)
• ✅ Good separation of concerns: IO devices, writer, and detector classes
• ✅ Comprehensive test coverage for the migration
• ✅ Consistent with existing ophyd-async patterns
• ✅ Tests properly updated to use FastCS PVs instead of EPICS

Recommendations

1. Clarify the intent of fp.frames.set(0) - add a comment or reconsider if frame count should be tracked
2. Rename od → odin in EigerDetector
3. Remove unnecessary single-item gather() calls
4. Track the FastCS Jungfrau update to remove the duplicate OdinHdfIO class

---

[claude]

Setting frames to 0 appears unusual. What does fp.frames represent in the Odin context?

If it's the expected frame count, this would always set it to 0 regardless of acquisition parameters. If it's a frame counter that should be reset, consider adding a comment explaining this behavior.

[claude]

Suggested change

	await asyncio.gather(
	await wait_for_value(self._drv.fp.writing, True, timeout=DEFAULT_TIMEOUT)

Single-item gather() is unnecessary.

[claude]

Suggested change

	od: OdinHdfIO
	odin: OdinHdfIO

As mentioned in previous review, odin is clearer than the abbreviated od.

[claude]

This duplicate OdinHdfIO class should be tracked for removal. Once FastCS Jungfrau provides the same structure as Eiger (with top-level 'detector' and 'odin'), this workaround can be eliminated.

Consider creating a GitHub issue to track this technical debt if one doesn't already exist.

[coretl]

Merged into #1177