Skip to content

Add Eiger stream2 support #112

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 7 commits into
base: eiger-api
Choose a base branch
from
Draft

Add Eiger stream2 support #112

wants to merge 7 commits into from

Conversation

GDYendell
Copy link
Contributor

@GDYendell GDYendell commented Aug 23, 2024 •
edited
Loading

@codecov Codecov
Copy link

codecov bot commented Aug 23, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 90.18405% with 16 lines in your changes missing coverage. Please review.

Project coverage is 94.26%. Comparing base (76c6b76) to head (995fd08).

Files with missing lines Patch % Lines
src/tickit_devices/eiger/stream/stream2.py 31.81% 15 Missing ⚠️
src/tickit_devices/eiger/stream/eiger_stream_2.py 98.52% 1 Missing ⚠️
Additional details and impacted files 
@@              Coverage Diff              @@
##           eiger-api     #112      +/-   ##
=============================================
- Coverage      94.74%   94.26%   -0.49%     
=============================================
  Files             34       36       +2     
  Lines           1751     1883     +132     
=============================================
+ Hits            1659     1775     +116     
- Misses            92      108      +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@GDYendell GDYendell force-pushed the eiger-api branch 5 times, most recently from 4db4ceb to 77ea6fe Compare August 29, 2024 12:34
@GDYendell GDYendell force-pushed the eiger-api branch 2 times, most recently from 016d5de to 262479b Compare August 29, 2024 13:05
@GDYendell GDYendell mentioned this pull request Sep 27, 2024
Closed
@jsouter
Fix eiger put responses to reflect real detector behaviour (Fixes #114)…
ce7a7f8 
… (#115)

Return correct changed parameters list for eiger detector put requests
Return empty list for puts to non detector eiger API paths to reflect real API
Test response of get_changed_parameters serialises properly
@GDYendell
Copy link
Contributor Author

GDYendell commented Nov 15, 2024 •
edited
Loading

It looks like photon_energy is supposed to return a list of changed parameters, but doesn't. We should check the 2024.1 firmware fixes this and update the simulator response.

See: areaDetector/ADEiger#41

@jsouter
Copy link
Collaborator

jsouter commented Nov 20, 2024

It looks like photon_energy is supposed to return a list of changed parameters, but doesn't. We should check the 2024.1 firmware fixes this and update the simulator response.

See: areaDetector/ADEiger#41

The testing I did for ce7a7f8 was done against B21's Eiger 2 1M and that gave back a list of parameters for photon_energy, I believe that's on release-2022.1.1

@shihab-dls
Convert default values to correct type
995fd08 
Fastcs-eiger raises a valueerror for a few attributes, as they default to ints but are floats.
@GDYendell
Copy link
Contributor Author

GDYendell commented Apr 14, 2025 •
edited
Loading

There are some TODOs in the stream2 implementation that need a real detector to compare with to get right, so I suggest we get what we can into #111 and merge it and leave this one for now. @shihab-dls could you take a look at this PR and try to pull the fixes that are unrelated to the new stream2 api into #111?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GDYendell @jsouter @shihab-dls