Skip to content
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.

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

Devel #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Devel #3

wants to merge 2 commits into from

Conversation

gjover
Copy link

@gjover gjover commented Feb 12, 2018

This pull request includes two commits.

The first fix sets the detector in ready state after stopping an acquisition.

The second fix allows to discover if setroi command is available on Camera construction.
If the command is no available the Interface do not add ROI capability

Copy link
Contributor

@sebastien-petitdemange sebastien-petitdemange left a comment

Choose a reason for hiding this comment

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

Hi Gabriel,

Thanks for your contribution, I just have few questions...

//-----------------------------------------------------
//
//-----------------------------------------------------
void Camera::_checkcmd()
Copy link
Contributor

Choose a reason for hiding this comment

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

The available commands were already checked in Camera::_run, so why you need this extra function?

Copy link
Author

Choose a reason for hiding this comment

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

Commands availability has to be checked when the camera is constructed.
Otherwise other objects may get wrong capabilities.
I use the first communications with the hardware to do the check, this is in connect method which has a lock to avoid conflicts with Camera::_run.
I guess that once it is checked on camera construction it is not needed on Camera::_run method.

In my case, 1M pilatus3 does not have setroi command. If this is discovered after creating the Interface Lima thinks that hardware roi exists and crashes.

{
DEB_MEMBER_FUNCT();
char messages[16384];
int aMessageSize = recv(m_socket,messages,sizeof(messages),0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will conflicts with the recv in the thread of Camera::_run.

Copy link
Author

Choose a reason for hiding this comment

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

Camera::_checkcmd is called in Camera::connect and it has a lock that prevents from communication conflicts.

Camera::_checkcmd justs reads the answers received from Camera::_resync calls.

PD1: In fact, it may be worth to move _resync from Camera::_connect to Camera::connect.
PD2: It may also be worth to merge _resync and _checkcmd, but this depends on it's use in _run and _reinit

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, It's more clear for me now...
I can merge your code as it is or with the modification you suggest, it's up to you, just tell me.
But could you remove the code in Camera::_run which check the command availability? to avoid any mistake.
Thanks again for your contribution.
P.S: You need to rebase on our master to fix conflict.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I've look a bit more deeper and I think it's safer to let the Camera::_run method do the socket reading because the _checkcmd may "eat" some other reply.
So to fix the bug you notice (Commands availability has to be checked when the camera is constructed), may I proposed you a patch (https://github.com/esrf-bliss/Lima-camera-pilatus/tree/wait_checkcmd). I can't tested it now, but the idea is to synchronize the connection with the reply of roi and setenergy command.
Tell me if it suit your need.

SEB

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