Update diagnostic to make it more general #998
Conversation
|
Hi @MSECode Remember to manage versioning. |
MSECode
force-pushed
the
feature/fixDiagnosticAEA
branch
from
cc08c6e to
7a15add
Compare
| The default value is the empty string | ||
| It returns false in case of error and the encoderTypeName string is filled with "ERROR" | ||
| */ | ||
| virtual bool getEncoderTypeName(uint32_t jomoId, eOmc_position_t pos, std::string &encoderTypeName); |
There was a problem hiding this comment.
Hi @MSECode, the class IethResource was created just to hold methods common to every EthResource, so it it should not contain a method that is related only to the MC device and not to for instance the SKIN device.
If you want something that is meaningful to MC device only you should use embObjMotionControl directly.
The way we use IethResource allows to easily transform IethResource* into its parent device.
The lines 44 and 45 tell briefly how to do that for the case of embObjSkin. I will show you in the next comment how to do that for the case of embObjMotionControl .
So, in here this code should be removed.
| @@ -75,6 +75,12 @@ bool IethResource::getEntityName(uint32_t entityId, std::string &entityName) | |||
| return true; | |||
| } | |||
|
|
|||
| bool IethResource::getEncoderTypeName(uint32_t jomoId, eOmc_position_t pos, std::string &encoderTypeName) | |||
There was a problem hiding this comment.
as from previous comment this code should be removed.
| @@ -447,6 +447,7 @@ class yarp::dev::embObjMotionControl: public DeviceDriver, | |||
| virtual eth::iethresType_t type(); | |||
| virtual bool update(eOprotID32_t id32, double timestamp, void *rxdata); | |||
| virtual bool getEntityName(uint32_t entityId, std::string &entityName); | |||
| virtual bool getEncoderTypeName(uint32_t jomoId, eOmc_position_t pos, std::string &encoderTypeName); | |||
There was a problem hiding this comment.
as from previous comments the virtual keyword should be removed
MSECode
force-pushed
the
feature/fixDiagnosticAEA
branch
from
c415131 to
82f6f37
Compare
There was a problem hiding this comment.
Some suggestions for resolving the CI failure
cc @marcoaccame
| @@ -94,7 +94,8 @@ add_library(${PROJECT_NAME} ${embobj_source} ${embobj_header}) | |||
|
|
|||
| include_directories(${CMAKE_CURRENT_SOURCE_DIR} ${CMAKE_CURRENT_SOURCE_DIR}/../motionControlLib | |||
| ${PATH_TO_CALLBACK}/embObjAnalog/usrcbk/ | |||
| ../skinLib) | |||
| ../skinLib | |||
| ../embObjMotionControl) | |||
There was a problem hiding this comment.
The CI failure is due to the fact that here we are adding embObjMotionControl as build dependency of embObjLib, but embObjMotionControl depends from embObjLib, creating a circular dependency we should avoid.
| return false; | ||
| } | ||
|
|
||
| if(embObjMotionControl *emc = dynamic_cast<embObjMotionControl*>(m_MC_ethRes); emc != nullptr) |
There was a problem hiding this comment.
To avoid circular dependency I would:
- add
getEncoderTypeNamein theIEthResourcewith a default implementation - Implement (marking it
override)getEncorderTypeNameinembObjMotionControl - Change this code in order to directly invoke
m_MC_ethRes->getEncoderTypeNamewithout the need of the dynamic_cast
Make for abs encoders (at joint) are more general Moroever, now we show the abs encoder type too in the diagnostic message Aligned with icub-firmware-shared 1.41.1
MSECode
force-pushed
the
feature/fixDiagnosticAEA
branch
from
be15947 to
1885e14
Compare
|
The CI is now happy. |
|
Awaiting the final approval of @marcoaccame and @valegagge before merging. |
|
/remind December 16, please let's proceed asap with this. |
|
⏰ Reminder
|
|
Great! Let me merge it then as we need to address this failure: Also, #989 will have to be rebased on top of this because of versioning. Tip Further requests can be integrated later on, possibly. |
|
Make for abs encoders (at joint) are more general
Moroever, now we show the abs encoder type too in the diagnostic message, solving the issue: robotology/icub-firmware#533
Related to: