Skip to content

Commands bug fix #65

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
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Commands bug fix #65

wants to merge 3 commits into from

Conversation

@AlvaroEzq
Copy link
Collaborator

@AlvaroEzq AlvaroEzq commented Sep 6, 2024 •
edited
Loading

Fixing a couple of caen devices commands that didn't work.

  • channel.stat (commit 4bea0b4)
  • module.board_alarm_status (commit 0fb875d)
    The problem on both relies on the usage of the check_command_output_and_convert function, where the method argument is not the correct one.
    This PR fixes both commands with ad-hoc commits. It would be better to have a consistent usage of the check_command_output_and_convert function with the commands mapping, but for now this works...

@lobis
Copy link
Owner

lobis commented Oct 16, 2024

Hi @AlvaroEzq , let's try to merge this. Is this PR complete or are there more needed changes? I think we would need to test these changes against real hardware to see if they work. I think we should merge this as soon as this is done (maybe it's done already).

@AlvaroEzq
Copy link
Collaborator Author

AlvaroEzq commented Oct 19, 2024

Hi @AlvaroEzq , let's try to merge this. Is this PR complete or are there more needed changes? I think we would need to test these changes against real hardware to see if they work. I think we should merge this as soon as this is done (maybe it's done already).

I have been using this branch for a long time now with real hardware and it works fine. Maybe it would be better to have a consistent usage of the check_command_output_and_convert function with the commands mapping instead of these ad-hoc fixes but it works with no problems.

@AlvaroEzq AlvaroEzq marked this pull request as ready for review October 19, 2024 16:38
@AlvaroEzq AlvaroEzq requested a review from lobis as a code owner October 19, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lobis lobis Awaiting requested review from lobis lobis is a code owner

At least 1 approving review is required to merge this pull request.

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

@AlvaroEzq @lobis