[NOT FOR MERGE] Feature/coe parametrization #12

MagnaboscoL · 2016-10-20T08:07:35Z

The requested changes have been implemented.
Now RTT knows the enum ec_state and its constants values
A new function "checkNetworkState" has been introduced in order to wrap soem api that check slaves' state. The function is aimed to introduce the following improvements:
not only master state but also slaves' state is updated after that a state change has been verified
the code seems more compact

TODO before merging:

test with last drivers version
check for backward compatibility

* add soem_master_types.hpp * add the "parameter" type to soem_master * add the "ec_state" type to soem_master * minor bugfix: state of the slaves properly updated

Conflicts: soem_master/soem_master_component.cpp soem_master/soem_master_component.h soem_master/soem_master_types.hpp

* add soem_master_types.hpp * updated files containing features * minor space canges in soem_driver.h * tab alignment fix in soem_master_component * white spaces fixing * fixing marked issues for pr * fix for pr

Conflicts: soem_master/soem_master_component.cpp soem_master/soem_master_component.h soem_master/soem_master_types.hpp

smits · 2016-10-20T08:12:57Z

ptal @adolfo-rt, @mvukov

MagnaboscoL · 2016-10-20T13:12:27Z

in soem_master_component.cpp I introduced a bug:
the for statements to cycle through the slaves should be "for(int i = 1; i <= ec_slavecount; i++)"

adolfo-rt

90% of comments are style and nits.

It would be good that someone with good EtherCAT-fu takes a pass as well.

adolfo-rt · 2016-10-20T13:23:46Z

soem_master/soem_master_component.cpp

@@ -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(


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

adolfo-rt · 2016-10-20T13:29:13Z

soem_master/soem_master_component.cpp

+ "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) );


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

adolfo-rt · 2016-10-20T13:31:29Z

soem_master/soem_master_component.cpp

+
+ 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>"));


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.

adolfo-rt · 2016-10-20T13:35:00Z

soem_master/soem_master_component.cpp

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


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

adolfo-rt · 2016-10-20T13:40:14Z

soem_master/soem_master_component.cpp

+ tmp.index = parameters[i].index;
+ tmp.sub_index = parameters[i].sub_index;
+
+ if(ec_slave[tmp.slave_position].mbx_proto & ECT_MBXPROT_COE)


style: space after if

adolfo-rt · 2016-10-20T14:34:59Z

soem_master/soem_master_component.h

@@ -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;


style: No space after std::vector.

adolfo-rt · 2016-10-20T14:37:14Z

soem_master/soem_master_types.hpp

+}
+
+};
+


style: Only one newline at end.

adolfo-rt · 2016-10-20T14:41:23Z

soem_master/soem_driver.h

@@ -45,6 +45,8 @@ extern "C"

 #include <sstream>

+#include "soem_master_types.hpp"


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

adolfo-rt · 2016-10-20T14:54:05Z

soem_master/soem_master_types.hpp

+ /** Index of the CoE object */
+ int index;
+ /** Subindex of the CoE object */
+ int sub_index;


Why are different types used here and in the AddressInfo struct? int vs unsigned int vs char.

Here implicit conversions are taking place, and here the char sub_index is being cast to int. If possible, it would be best to use types that need no casting, implicit or explicit.

Right, it this was because I wasn't sure.
In fact there are thresholds that these members should respect

for slave_position : max 65535 maximum theoretical number of slaves addressed by a single

for index: max 65535 because of the limit in the object dictionary entries

for sub_index: max 255
In fact in the soem header ethercoe.h
ec_SDOwrite(uint16 Slave, uint16 Index, uint8 SubIndex, ...)

So my doubt are:

use the right number of bytes as soem does?

keep it simple and use an int?

Opinions?

Keep it consistent above all: soem types and no casts, or ints and explicit casts when calling soem, but not a mix. The value ranges for each member can also be documented.

Notice that with default compiler flags, you can still assign -1 to an unsigned int and get an overflow, so no protection from out-of-range values. Adding -Wconversion -Werror (in gcc) enables additional compiler checks that would generate a compile error.

OK,
setting the members of the Parameter structure to be SOEM types would lead to some errors while parsing soem.cpf such as:
"[ ERROR ][TinyDemarshaller] Unknown type "uint16" for for tag simple"
and at the moment I do not know how to fix that, so I keep these members as "int" while an explicit cast is done to the SOEM type while saving in "AddressInfo" structure.

PS
The "AddressInfo" structure has been introduced because I had compilation error calling "this->addOperation("writeCoeSdo,..." for a function with more than 5 arguments.

We'll probable need the stdint typekit to resolve this? Can you try and see if these errors disappear if you load this typekit. (https://github.com/orocos-toolchain/stdint_typekit)

Well what was already done (not committed yet) was:
In soem_master_types.hpp
#include <rtt/typekit/StdTypeInfo.hpp>

In soem_master_component.cpp:
// To teach RTT SOEM fixed length types
RTT::types::Types()->addType(new types::StdTypeInfo<uint8_t>("uint8"));
RTT::types::Types()->addType(new types::StdTypeInfo<uint16_t>("uint16"));
RTT::types::Types()->addType(new types::StdTypeInfo<uint32_t>("uint32"));

But even doing that I could't load uin16 from the soem.cpf.
Maybe I have to add something else...when you write load you mean something different?

adolfo-rt · 2016-10-20T14:54:25Z

soem_master/soem_master_types.hpp

+};
+}
+
+//################################################################


style: Remove these comments.

adolfo-rt · 2016-10-20T14:58:41Z

@MagnaboscoL, to correctly display checkboxes in the description, the syntax is:

- [ ] incomplete
- [x] completed

incomplete
completed

mvukov

@adolfo-rt already made comments about style, I have more ECAT relevant questions/remarks.

mvukov · 2016-10-20T17:08:06Z

soem_master/soem_master_component.cpp

+
+ if(wkc == 0)
+ {
+ log(Error) << "Slave_" << ec_slave[tmp.slave_position].configadr <<" SDOwrite{index["<< tmp.index


IMO, in this case you should return false.

mvukov · 2016-10-20T17:16:09Z

soem_master/soem_master_component.cpp

@@ -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)


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?

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.

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

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

smits

Nice work, it will however not be ABI backwards compatible, so we'll need to bump the minor version number as well with this change.

smits · 2016-10-21T10:42:47Z

soem_master/soem_master_component.cpp

+
+ if(wkc == 0)
+ {
+ log(Error) << "Slave_" << ec_slave[tmp.slave_position].configadr <<" SDOwrite{index["<< tmp.index


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

smits · 2016-10-21T10:43:12Z

soem_master/soem_master_component.cpp

@@ -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();


80-char wrapping.

smits · 2016-10-21T10:44:25Z

soem_master/soem_master_component.cpp

- }
- //return false;
- }
+ checkNetworkState(EC_STATE_SAFE_OP, EC_TIMEOUTSTATE);


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

smits · 2016-10-21T10:45:55Z

soem_master/soem_master_component.cpp

@@ -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)


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.

smits · 2016-10-21T10:49:45Z

soem_master/soem_master_types.hpp

+ /** Index of the CoE object */
+ int index;
+ /** Subindex of the CoE object */
+ int sub_index;


We'll probable need the stdint typekit to resolve this? Can you try and see if these errors disappear if you load this typekit. (https://github.com/orocos-toolchain/stdint_typekit)

MagnaboscoL · 2016-10-21T12:18:15Z

OK I just pushed a commit that:

fix the style issues (C++ code style loaded in eclipse)
update CoE functions management as required
add two functions setSlavesTargetState and notifySoemErrors to try to have a code easier to be understood without knowing how SOEM library works
rename updateNetworkState to checkSlavesStateReachedWaiting

MagnaboscoL and others added 12 commits October 14, 2016 14:54

add soem_master_types.hpp

e881a13

updated files containing features

72ef26e

minor space canges in soem_driver.h

e3a1f7a

tab alignment fix in soem_master_component

95b61fd

white spaces fixing

c4b7c2f

Feature/coe parametrization (#1)

59de91a

* add soem_master_types.hpp * add the "parameter" type to soem_master * add the "ec_state" type to soem_master * minor bugfix: state of the slaves properly updated

fixing marked issues for pr

749a4d8

fix for pr

2170278

Merge branch 'master' into feature/coe_parametrization

0194536

Conflicts: soem_master/soem_master_component.cpp soem_master/soem_master_component.h soem_master/soem_master_types.hpp

Feature/coe parametrization (#2)

ee637f7

* add soem_master_types.hpp * updated files containing features * minor space canges in soem_driver.h * tab alignment fix in soem_master_component * white spaces fixing * fixing marked issues for pr * fix for pr

improved enum ec_state management

40615fa

Merge branch 'master' of github.com:MagnaboscoL/rtt_soem

9970b65

Conflicts: soem_master/soem_master_component.cpp soem_master/soem_master_component.h soem_master/soem_master_types.hpp

minor bug fix

f7ccbfe

adolfo-rt suggested changes Oct 20, 2016

View reviewed changes

mvukov reviewed Oct 20, 2016

View reviewed changes

smits requested changes Oct 21, 2016

View reviewed changes

fix style and SDO functions management

65bbbd5

style fixed

88f0907

		@@ -45,6 +45,8 @@ extern "C"

		#include <sstream>

		#include "soem_master_types.hpp"

[NOT FOR MERGE] Feature/coe parametrization #12

Are you sure you want to change the base?

[NOT FOR MERGE] Feature/coe parametrization #12

Conversation

MagnaboscoL commented Oct 20, 2016 • edited Loading

smits commented Oct 20, 2016

MagnaboscoL commented Oct 20, 2016

adolfo-rt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adolfo-rt commented Oct 20, 2016

mvukov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MagnaboscoL commented Oct 21, 2016

MagnaboscoL commented Oct 20, 2016 •

edited

Loading