Less copy paste

[facontidavide]

Hi,

I would like to propose these changes which remove several lines of copied and pasted (and error prone) code.
Note that I haven't had the opportunity to test it yet, but I want to start discussing if you agree from a "philosophical" point of view with these changes ;)

1. Make getter members const whenever you can.
2. Use the template specialization of unpackVariable to remove the manual offsets spread into the code.

Merge may come once we broadly tested this on the actual robot.

Davide

[NikolasE]

Nice code, I just wrote similar functions two weeks ago: https://github.com/NikolasE/ur_modern_driver/commit/13dc20e8549e1d149a8e8529598005e710ecc7ed (double parseDouble...) :)
Did you test your code on a real robot? I could parse everything but the float values.

[facontidavide]

Honestly I didn't check if float works. I just know that my code is SUPPOSED to do what the prvioud code does.
This doesn't mean that maybe the old code is working correctly.
I will check this afternoon on the UR5

[facontidavide]

By the way. It is nice to see that more people feel that a similar solution would be clearer.
My two cents. In my implementation I let the C++ type system infer the correct implementation through const reference.

This in my (humble) opinion is safer and less error prone, because otherwise no one prevent you to use parseDouble on a variable that is supposed to be an int.
Of course, even my method is not "bullet proof" because you might pass the reference to something with the wrong type :P

I guess it is a subjective decision.

I noticed also that you had problems as well with formatting, which makes your commit more "dirty". We should start using automated formatting as I suggested in #89

[NikolasE]

I tested your float-function and it fails for my UR10 with CB3.

tool_voltage_48V: 3458792192.0

[facontidavide]

does the original code (master) work? I will double check during the weekend o spot what is different

[NikolasE]

it did not work for me. The other parsing functions are working, but not the float

[facontidavide]

sorry, I am not sure I understood. Are you confirming that the float parsing is wrong as well in the master branch?

The double version (that works), can be re-written as

void RobotState::unpackVariable<double>(const uint8_t *buffer, unsigned int& offset, double& dest){
	uint64_t temp;
        memcpy(&temp, &buffer[offset], sizeof(temp));
	offset += sizeof(temp);
	temp = be64toh(temp);
	memcpy(&dest, &temp, sizeof(dest));
}

Therefore I assume that the correct implementation for float would be

void RobotState::unpackVariable<float>(const uint8_t *buffer, unsigned int& offset, float& dest){
	uint32_t temp;
        memcpy(&temp, &buffer[offset], sizeof(temp));
	offset += sizeof(temp);
	temp = be32toh(temp);
	memcpy(&dest, &temp, sizeof(dest));
}

I can not use the robot today. Can you test it? If you can't, I will check this out on Monday

[facontidavide]

By the way, I will move these methods to free standing functions in another file, since they can be reused to parse robot_state_RT.cpp too.
They were mean to be static methods anyway.

[NikolasE]

I started with the functions I extracted from the master and did not get useful values for the floats. I just ran your code (with the be32toh) and got this output:

rostopic echo /ur_driver/tool_data

analog_input_range2: 1
analog_input_range3: 1
analog_input2: 0.0482482016087
analog_input3: 0.0473143681884
tool_voltage_48V: 461.459228516
tool_output_voltage: 205
tool_current: 0.00300000002608
tool_temperature: 51.0
tool_mode: 253

(tool_data is only available in my fork I think)

The tool current and temperature are correct, (no idea what the tool_voltage_48V means and I'm not sure about the uchar-value of 205 for the output_voltage).

So I think this float-function is correct.

[facontidavide]

Is there anything that prevent us from merging this PR?