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

Jump to bottom

[NOT FOR MERGE] Feature/coe parametrization #12

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

[NOT FOR MERGE] Feature/coe parametrization #12

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e881a13
add soem_master_types.hpp
MagnaboscoL Oct 14, 2016
72ef26e
updated files containing features
MagnaboscoL Oct 14, 2016
e3a1f7a
minor space canges in soem_driver.h
MagnaboscoL Oct 14, 2016
95b61fd
tab alignment fix in soem_master_component
MagnaboscoL Oct 14, 2016
c4b7c2f
white spaces fixing
MagnaboscoL Oct 14, 2016
59de91a
Feature/coe parametrization (#1)
MagnaboscoL Oct 14, 2016
749a4d8
fixing marked issues for pr
MagnaboscoL Oct 18, 2016
2170278
fix for pr
MagnaboscoL Oct 18, 2016
0194536
Merge branch 'master' into feature/coe_parametrization
MagnaboscoL Oct 18, 2016
ee637f7
Feature/coe parametrization (#2)
MagnaboscoL Oct 18, 2016
40615fa
improved enum ec_state management
MagnaboscoL Oct 20, 2016
9970b65
Merge branch 'master' of github.com:MagnaboscoL/rtt_soem
MagnaboscoL Oct 20, 2016
f7ccbfe
minor bug fix
MagnaboscoL Oct 20, 2016
65bbbd5
fix style and SDO functions management
MagnaboscoL Oct 21, 2016
88f0907
style fixed
MagnaboscoL Oct 21, 2016
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
22 changes: 22 additions & 0 deletions soem_master/soem_driver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ extern "C"

#include <sstream>

#include "soem_master_types.hpp"
Copy link

Choose a reason for hiding this comment

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

Not needed in this header? but in soem_master_component.h instead?

Copy link
Author

Choose a reason for hiding this comment

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

Done


template<class T>
inline std::string to_string(const T& t, std::ios_base & (*f)(std::ios_base&))
{
Expand Down Expand Up @@ -105,6 +107,22 @@ class SoemDriver
return (ec_state)(m_datap->state);
};

virtual int getInputByteLength(){
return (int)(m_datap->Ibytes);
};

virtual int getOutputByteLength(){
return (int)(m_datap->Obytes);
};

virtual int getInputBitLength(){
return (int)(m_datap->Ibits);
};

virtual int getOutputBitLength(){
return (int)(m_datap->Obits);
};

protected:
SoemDriver(ec_slavet* mem_loc) :
m_datap(mem_loc), m_name("Slave_" + to_string(m_datap->configadr,
Expand All @@ -114,6 +132,10 @@ class SoemDriver
m_service->addOperation("checkState",&SoemDriver::checkState,this).doc("check the slaves state").arg("state","state value to check");
m_service->addOperation("getState",&SoemDriver::getState,this).doc("request slave state");
m_service->addOperation("configure",&SoemDriver::configure,this).doc("Configure slave");
m_service->addOperation("getInputByteLength",&SoemDriver::getInputByteLength,this).doc("retrieve the length of the input cyclic data in bytes");
m_service->addOperation("getOutputByteLength",&SoemDriver::getOutputByteLength,this).doc("retrieve the length of the output cyclic data in bytes");
m_service->addOperation("getInputBitLength",&SoemDriver::getInputBitLength,this).doc("retrieve the length of the input cyclic data in bits");
m_service->addOperation("getOutputBitLength",&SoemDriver::getOutputBitLength,this).doc("retrieve the length of the output cyclic data in bits");
}
;
ec_slavet* m_datap;
Expand Down
205 changes: 149 additions & 56 deletions soem_master/soem_master_component.cpp
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,29 @@ SoemMasterComponent::SoemMasterComponent(const std::string& name) :
"Second (redundant) interface to which the ethercat device is connected");
this->addProperty("redundant", prop_redundant=false).doc(
"Whether to use a redundant nic");
this->addProperty("slavesCoeParameters",parameters).doc(
Copy link

Choose a reason for hiding this comment

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

style: space after comma, here and for added operations below.

Copy link
Author

Choose a reason for hiding this comment

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

Done

"Vector of parameters to be sent to the slaves using CoE SDO");

SoemDriverFactory& driver_factory = SoemDriverFactory::Instance();
this->addOperation("displayAvailableDrivers",
&SoemDriverFactory::displayAvailableDrivers, &driver_factory).doc(
"display all available drivers for the soem master");
this->addOperation("writeCoeSdo", &SoemMasterComponent::writeCoeSdo,this).doc(
"send a CoE SDO write (blocking: not to be done while slaves are in OP)");
this->addOperation("readCoeSdo", &SoemMasterComponent::readCoeSdo,this).doc(
"send a CoE SDO read (blocking: not to be done while slaves are in OP)");

RTT::types::GlobalsRepository::shared_ptr globals = RTT::types::GlobalsRepository::Instance();
globals->setValue( new Constant<ec_state>("EC_STATE_INIT",EC_STATE_INIT) );
Copy link

Choose a reason for hiding this comment

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

style: No space after ( nor before ), add space after comma. Here and below.

Copy link
Author

Choose a reason for hiding this comment

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

Done

globals->setValue( new Constant<ec_state>("EC_STATE_PRE_OP",EC_STATE_PRE_OP) );
globals->setValue( new Constant<ec_state>("EC_STATE_SAFE_OP",EC_STATE_SAFE_OP) );
globals->setValue( new Constant<ec_state>("EC_STATE_OPERATIONAL",EC_STATE_OPERATIONAL) );
globals->setValue( new Constant<ec_state>("EC_STATE_BOOT",EC_STATE_BOOT) );

RTT::types::Types()->addType(new ec_stateTypeInfo());
RTT::types::Types()->addType(new parameterTypeInfo());
RTT::types::Types()->addType(new types::SequenceTypeInfo< std::vector<rtt_soem::Parameter> >("std.vector<Parameter>"));
Copy link

Choose a reason for hiding this comment

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

style: wrap line at 80 cols. Applies to other long lines in the diff. No need to fix existing code violating this, but new code should strive to comply with our style guide.

Copy link
Author

Choose a reason for hiding this comment

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

OK


//this->addOperation("start",&TaskContext::start,this,RTT::OwnThread);
}

Expand Down Expand Up @@ -91,8 +110,38 @@ bool SoemMasterComponent::configureHook()
<< endlog();
ec_slave[0].state = EC_STATE_PRE_OP;
ec_writestate(0);
// wait for all slaves to reach PRE_OP state
ec_statecheck(0, EC_STATE_PRE_OP, EC_TIMEOUTSTATE);
//Wait for all slaves to reach PRE_OP state
Copy link

Choose a reason for hiding this comment

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

style: Space after //, here and in other comments.

Copy link
Author

Choose a reason for hiding this comment

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

OK

if(!checkNetworkState(EC_STATE_PRE_OP, EC_TIMEOUTSTATE))
return false;

//The parameters to be sent to the slaves are loaded from the soem.cpf:
//parameters could be changed without modifying the code
for (unsigned int i=0; i < parameters.size(); i++)
{
int wkc;
Copy link

Choose a reason for hiding this comment

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

Consider giving wkc and tmp more descriptive names.

Copy link
Author

Choose a reason for hiding this comment

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

I agree but wkc is the standard name for the "working counter" in the EtherCAT specifications and in few EtherCAT libraries such as SOEM and Etherlab.
I would add a comment just above the variable saying:
// Working counter of the EtherCAT datagram that ends the CoE download procedure
// This value is written into the frame by the slave and is used to confirm that
// the command has been executed. In this case the expected working counter is 1.

Is it enough in your opinion? Or should I change it anyway?

Copy link

Choose a reason for hiding this comment

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

If this is a well understood acronym in this context, it can remain as-is.

Copy link

Choose a reason for hiding this comment

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

For the sake of clarity, I would use working_counter.

Copy link
Author

Choose a reason for hiding this comment

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

OK

AddressInfo tmp;
tmp.slave_position = parameters[i].slave_position;
tmp.index = parameters[i].index;
tmp.sub_index = parameters[i].sub_index;

if(ec_slave[tmp.slave_position].mbx_proto & ECT_MBXPROT_COE)
Copy link

Choose a reason for hiding this comment

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

style: space after if

Copy link
Author

Choose a reason for hiding this comment

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

Done

{
wkc = writeCoeSdo(tmp,parameters[i].complete_access,parameters[i].size,&parameters[i].param);

if(wkc == 0)
Copy link

Choose a reason for hiding this comment

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

style: space after if

Copy link
Author

Choose a reason for hiding this comment

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

Done

{
log(Error) << "Slave_" << ec_slave[tmp.slave_position].configadr <<" SDOwrite{index["<< tmp.index
Copy link

Choose a reason for hiding this comment

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

Should configuration fail on error? If not (best effort?), consider lowering severity to warning. Consider also documenting on the header the circumstances under which configure can fail.

Copy link

Choose a reason for hiding this comment

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

IMO, in this case you should return false.

Copy link
Author

Choose a reason for hiding this comment

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

OK

Copy link
Member

Choose a reason for hiding this comment

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

+1 on returning false here, we're still configuring, if things go wrong here, don't expect anything to work.

<< "] sub_index["<< (int)tmp.sub_index <<"] size "<< parameters[i].size
<< " value "<< parameters[i].param << "} wkc "<< wkc << endlog();
}
}
else
{
log(Error) << "Slave_" << ec_slave[tmp.slave_position].configadr <<" does not support CoE"
<< " but in soem.cpf there is a CoE parameter for this slave." << endlog();
}

}

for (int i = 1; i <= ec_slavecount; i++)
{
Expand Down Expand Up @@ -142,6 +191,7 @@ bool SoemMasterComponent::configureHook()
else
{
log(Error) << "Configuration of slaves failed!!!" << endlog();
log(Error) << "The NIC currently used for EtherCAT is "<< prop_ifname1.c_str() << " . Another could be chosen by editing soem.cpf." << endlog();
Copy link

Choose a reason for hiding this comment

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

nit: If msg is part of the above error, prefer using a newline instead of two separate log statements.

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

80-char wrapping.

return false;
}
return true;
Expand All @@ -156,40 +206,13 @@ bool SoemMasterComponent::configureHook()

bool SoemMasterComponent::startHook()
{
bool state_reached;

log(Info) << "Request safe-operational state for all slaves" << endlog();
ec_slave[0].state = EC_STATE_SAFE_OP;
ec_writestate(0);
// wait for all slaves to reach SAFE_OP state
ec_statecheck(0, EC_STATE_SAFE_OP, EC_TIMEOUTSTATE);

if (ec_slave[0].state == EC_STATE_SAFE_OP)
{
log(Info) << "Safe operational state reached for all slaves."
<< endlog();
while (EcatError)
{
log(Error) << ec_elist2string() << endlog();
}
}
else
{
log(Error) << "Not all slaves reached safe operational state."
<< endlog();
ec_readstate();
//If not all slaves operational find out which one
for (int i = 0; i <= ec_slavecount; i++)
{
if (ec_slave[i].state != EC_STATE_SAFE_OP)
{
log(Error) << "Slave " << i << " State= " << to_string(
ec_slave[i].state, std::hex) << " StatusCode="
<< ec_slave[i].ALstatuscode << " : "
<< ec_ALstatuscode2string(
ec_slave[i].ALstatuscode) << endlog();
}
}
//return false;
}
checkNetworkState(EC_STATE_SAFE_OP, EC_TIMEOUTSTATE);
Copy link

Choose a reason for hiding this comment

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

+1 for less code dup :)

Copy link

Choose a reason for hiding this comment

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

The name of the method is a bit misleading, as a check is something that I would expect to not have side-effects. Consider:

  • Renaming it to something that reflects the performed actions, which are checking the state and updating the internal state.
  • Split the method in two, maybe: update + check (on the internal state?)

Copy link
Author

Choose a reason for hiding this comment

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

Well maybe I can rename the function to waitNetworkStateReached() since its goal is
wait for a certain amount of time if the desired state has been reached.

To achieve this goal the function does three things

  1. read the current state from the remote devices (slaves)
  2. check if the desired state is reached, if not and if timeout not elapsed comes back to step 1
  3. in any case update the value of the "state" in the structure "ec_slave[]"
    with the current state retrieved from the remote devices.

Copy link
Member

Choose a reason for hiding this comment

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

I would additionally replace Network with Slave --> updateSlaveStates or something similar.

Copy link
Author

Choose a reason for hiding this comment

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

So maybe "checkSlavesStateReachedWaiting" ?


log(Info) << "Request operational state for all slaves" << endlog();
ec_slave[0].state = EC_STATE_OPERATIONAL;
Expand All @@ -204,31 +227,9 @@ bool SoemMasterComponent::startHook()
}

// wait for all slaves to reach OP state
ec_statecheck(0, EC_STATE_OPERATIONAL, EC_TIMEOUTSTATE);
if (ec_slave[0].state == EC_STATE_OPERATIONAL)
{
log(Info) << "Operational state reached for all slaves."
<< endlog();
}
else
{
log(Error) << "Not all slaves reached operational state."
<< endlog();
//If not all slaves operational find out which one
for (int i = 1; i <= ec_slavecount; i++)
{
if (ec_slave[i].state != EC_STATE_OPERATIONAL)
{
log(Error) << "Slave " << i << " State= " << to_string(
ec_slave[i].state, std::hex) << " StatusCode="
<< ec_slave[i].ALstatuscode << " : "
<< ec_ALstatuscode2string(
ec_slave[i].ALstatuscode) << endlog();
}
}
return false;
if(!checkNetworkState(EC_STATE_OPERATIONAL, EC_TIMEOUTSTATE))
return false;

}
return true;
}

Expand Down Expand Up @@ -269,4 +270,96 @@ void SoemMasterComponent::cleanupHook()
//stop SOEM, close socket
ec_close();
}

int SoemMasterComponent::writeCoeSdo(const AddressInfo& address, bool complete_access, int size, void* data)
Copy link

Choose a reason for hiding this comment

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

As we discussed offline:

  • If this is blocking call, this method and corresponding read method should check the state: if operational, the call should not be made. The rationale: once operational, the communication enters cyclic mode. Thus, read/write of SDOs would possibly postpone PDOs.
  • The function should return bool: true on success, false otherwise.
  • However, as @MagnaboscoL indicated, SDOs are sometimes useful in operational state to handle emergency situations. Therefore, another solution here may be to make those functions protected.

@smits What do you think about this?

Copy link
Member

Choose a reason for hiding this comment

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

I would clearly indicate the behavior in the documentation of the function. I agree that the user should be allowed to to SDO writes when he feels like it, given that he knows what he's doing.

Copy link
Author

Choose a reason for hiding this comment

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

Ok,
for the moment I have modified the functions so that:

  • if at least a slave is in operational state they simply return false without trying to send an SDO.
  • return true in case it succeeded, false otherwise

Notes:

  • from the wireshark record I have seen that an SDO write can take up to 80 ms to be completed but it really depends on the slave. Since the procedure is completed only when the slave confirmed that the SDO has peen processed or when too much time has elapsed.
  • as far as I know SDO are allowed by the EtherCAT standard in the states preop, safeop and operational.
  • in my opinion an SDO can be used in operational at least in the following cases:
    • if a drives notify a fault using its PDO mapped statusword it can be useful to know which kind of fault it is and that could be done reading the proper object. Usually the standard object for this purpose is index: 0x1003 (subindex: 0 = fault count, other subindices = fault code)
    • if a drive controls also the brake motor, it can be useful to open the brake even if the drive is not enabled just to be able to move the axis by hand. This is usually done by SDO for Ds402 drives even though it would be possible to manage it mapping the needed object to a PDO

Copy link
Author

Choose a reason for hiding this comment

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

I didn't see the comment on time :)
ok so the functions could be called in operational too

{
return ec_SDOwrite(address.slave_position,address.index,address.sub_index,complete_access,size,data,EC_TIMEOUTRXM);
Copy link

Choose a reason for hiding this comment

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

style, here and below: 80 col wrap and space after comma.

Copy link
Author

Choose a reason for hiding this comment

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

Done

}

int SoemMasterComponent::readCoeSdo(const AddressInfo& address, bool complete_access, int* size, void* data)
{
return ec_SDOread(address.slave_position,address.index,address.sub_index,complete_access,size,data,EC_TIMEOUTRXM);
}

bool SoemMasterComponent::checkNetworkState(ec_state desired_state, int timeout)
{
bool state_is_reached = true;
bool error_detected = false;

uint16 network_state = ec_statecheck(0, desired_state, timeout);

if((network_state & 0xf0) == 0)
{
// No slave has toggled the error flag so the AlStatusCode (even if different from 0) should be ignored
for(int i = 0; i < ec_slavecount; i++)
{
ec_slave[i].ALstatuscode = 0x0000;
}
}
else
{
error_detected = true;
}

switch(network_state)
{
case EC_STATE_INIT:
case EC_STATE_PRE_OP:
case EC_STATE_BOOT:
case EC_STATE_SAFE_OP:
case EC_STATE_OPERATIONAL:
if(!error_detected)
{
//All the slaves have reached the same state so we can update the state of every slave
for(int i = 1; i <= ec_slavecount; i++)
{
ec_slave[i].state = network_state;
}
}
else
{
ec_readstate();
}
break;

default:
//The state should be verified for every single slave
//since not all have the same state
ec_readstate();
break;
}

if (ec_slave[0].state == desired_state)
{
log(Info) << (ec_state)ec_slave[0].state <<" state reached for all slaves."
<< endlog();
while (EcatError)
{
log(Error) << ec_elist2string() << endlog();
}
}
else
{
log(Error) << "Not all slaves reached safe operational state."
<< endlog();

//If not all slaves reached target state find out which one
for (int i = 1; i <= ec_slavecount; i++)
{
if (ec_slave[i].state != desired_state)
{
state_is_reached = false;

log(Error) << "Slave " << i << " State= " <<
(ec_state)ec_slave[i].state << " StatusCode="
<< ec_slave[i].ALstatuscode << " : "
<< ec_ALstatuscode2string(
ec_slave[i].ALstatuscode) << endlog();
}
}
}

return state_is_reached;
}

}//namespace
13 changes: 13 additions & 0 deletions soem_master/soem_master_component.h
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@
namespace soem_master
{

/** CoE addressing info */
struct AddressInfo
{
unsigned short slave_position;
unsigned short index;
unsigned char sub_index;
};

class SoemMasterComponent: public RTT::TaskContext
{
public:
Expand All @@ -50,6 +58,11 @@ class SoemMasterComponent: public RTT::TaskContext
bool prop_redundant;
char m_IOmap[4096];
std::vector<SoemDriver*> m_drivers;
std::vector <rtt_soem::Parameter> parameters;
Copy link

Choose a reason for hiding this comment

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

style: No space after std::vector.

Copy link
Author

Choose a reason for hiding this comment

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

Done

int writeCoeSdo(const AddressInfo& address, bool complete_access, int size, void* data);
Copy link

Choose a reason for hiding this comment

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

Add some doc on what the methods do, what the params represent, and the meaning of the return value.

Copy link
Author

Choose a reason for hiding this comment

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

Done

int readCoeSdo(const AddressInfo& address, bool complete_access, int* size, void* data);
bool checkNetworkState(ec_state desired_state, int timeout);

};//class

}//namespace
Expand Down
Loading