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.

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

Devel #3

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

Devel #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ba05e8
Fix Error status after stop acquisition
gjover Feb 6, 2018
12d29db
Fix to discover unavailable commands at Camera construction.
gjover Feb 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/PilatusCamera.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ class Camera
void _run();
void _initVariable();
void _resync();
void _checkcmd();
void _reinit();
void _pilatus2model();
void _pilatus3model(); ///< set pilatus3 threshold extention
Expand Down
59 changes: 53 additions & 6 deletions src/PilatusCamera.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,7 @@ void Camera::connect(const char *host,int port)
AutoMutex aLock(m_cond.mutex());
_initVariable();
_connect(host,port);
_checkcmd();
}

void Camera::_connect(const char *host,int port)
Expand Down Expand Up @@ -277,6 +278,7 @@ void Camera::_connect(const char *host,int port)
//-----------------------------------------------------
void Camera::_resync()
{
DEB_MEMBER_FUNCT();
if(m_has_cmd_setenergy)
send("setenergy");
else
Expand All @@ -296,6 +298,41 @@ void Camera::_resync()
send("version");
}

//-----------------------------------------------------
//
//-----------------------------------------------------
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

if(aMessageSize > 0)
{
std::string strMessages(messages,aMessageSize );
std::vector<std::string> msg_vector;
_split(strMessages,SPLIT_SEPARATOR,msg_vector);
for(std::vector<std::string>::iterator msg_iterator = msg_vector.begin();
msg_iterator != msg_vector.end();++msg_iterator)
{
std::string &msg = *msg_iterator;
if(msg.find("Unrecognized command: setenergy") !=
std::string::npos)
{
DEB_TRACE() <<"-- no setenergy command";
m_has_cmd_setenergy = false;
}
if(msg.find("Unrecognized command: setroi") != std::string::npos
or
msg.find("Invalid command: SetROI") != std::string::npos
)
{
DEB_TRACE() <<"-- no setroi command";
m_has_cmd_roi = false;
}
}
}
}

//-----------------------------------------------------
//
//-----------------------------------------------------
Expand Down Expand Up @@ -574,12 +611,17 @@ void Camera::_run()
m_state = Camera::STANDBY;
m_nb_acquired_images = m_nimages;
}
else if(m_state == Camera::STANDBY)
{
// The acquisition was aborted
DEB_TRACE() << "-- Ignore ERROR";
}
else
{
DEB_TRACE() << "-- ERROR";
m_state = Camera::ERROR;
msg = msg.substr(2);
m_error_message = msg.substr(msg.find(" "));
DEB_TRACE() << "-- ERROR";
m_state = Camera::ERROR;
msg = msg.substr(2);
m_error_message = msg.substr(msg.find(" "));
}
}
else if(msg.substr(0,2) == "1 ")
Expand All @@ -599,8 +641,13 @@ void Camera::_run()
DEB_TRACE() << "Can't retrieved camserver version";
}
else if(msg.find("Unrecognized command: setroi") !=
std::string::npos)
std::string::npos
or
msg.find("Invalid command: SetROI") !=
std::string::npos
)
{
DEB_TRACE() << "Does not have setroi cmd";
m_has_cmd_roi = false;
_resync();
}
Expand Down Expand Up @@ -1063,7 +1110,7 @@ void Camera::stopAcquisition()
if(m_state == Camera::RUNNING)
{
m_state = Camera::KILL_ACQUISITION;
send("k");
send("camcmd k");
}
}

Expand Down
11 changes: 9 additions & 2 deletions src/PilatusInterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,9 @@ RoiCtrlObj::RoiCtrlObj(Camera& cam,DetInfoCtrlObj& det) :
else
m_has_hardware_roi = false;

if(cam.hasRoiCapability() == false)
m_has_hardware_roi = false;

if(!m_has_hardware_roi)
DEB_WARNING() << "Hardware Roi not managed for this detector";

Expand Down Expand Up @@ -753,8 +756,12 @@ Interface::Interface(Camera& cam,const DetInfoCtrlObj::Info* info)
HwSavingCtrlObj *saving = &m_saving;
m_cap_list.push_back(HwCap(saving));

HwRoiCtrlObj *roi = &m_roi;
m_cap_list.push_back(HwCap(roi));
if (cam.hasRoiCapability() == true)
{
DEB_TRACE() << "Has hardware ROI capability";
HwRoiCtrlObj *roi = &m_roi;
m_cap_list.push_back(HwCap(roi));
}

m_buffer.getDirectoryEvent().watch_moved_to();

Expand Down