Extend motoRosAssert(..) to output expected value

[gavanderhoorn]

Current implementation posts an alarm and enters an infinite loop printing motoRosAssert: MotoROS2: Fatal Error (subcode: N).

While that works, it would be more helpful to us (and users) if it printed the expected value (whatever that would mean for specific motoRosAssert(..) invocations).

Example:

motoros2/src/PositionMonitor.c

Lines 55 to 62 in d8b75dd

	//create a publisher for aggregate joint state of all groups
	rcl_ret_t ret = rclc_publisher_init(
	&g_publishers_PositionMonitor.jointStateAllGroups,
	&g_microRosNodeInfo.node,
	ROSIDL_GET_MSG_TYPE_SUPPORT(sensor_msgs, msg, JointState),
	TOPIC_NAME_JOINT_STATES,
	qos_profile);
	motoRosAssert(ret == RCL_RET_OK, SUBCODE_FAIL_CREATE_PUBLISHER_JOINT_STATE_ALL);

an extended version of motoRosAssert(..) would output the values of ret and RCL_RET_OK as part of the message.

[gavanderhoorn]

Context: #266 (among others).

[gavanderhoorn]

This could have helped with #292 as well.

[jimmy-mcelwain]

What would the scope of this issue be? The conditions tested by motoRosAssert have a wide variety of operators. An initial search shows ==, !=, >=, <=, non-NULL, etc. are used. It could also cover several different types. Would we want a function for each of these checks? The implementation wouldn't be difficult, but I don't know where the best place to stop would be.

[gavanderhoorn]

We should be able to use macro stringification to avoid having to create N different versions of motoRosAssert(..).

There are various C / C++ unit testing frameworks which use this approach to print more informative result/error messages.

In the end motoRosAssert(1 < x, ...) would result in the 1 < x being available as a string itself, in addition to being a conditional. That string-ified version could then used as part of the message.

---

Edit: I just realised this won't help accessing the values that are use in the conditionals. So I would suggest we indeed create motoRosAssertEquals(..) et al.

I would really recommend to take a look at some existing C unit testing frameworks.

re: where to stop: let's just start with the minimal set needed to cover the current conditionals. We can always add more later.

[jimmy-mcelwain]

I have some work related to this here. I am using macro stringification to print out variable names and values. The macros make it a bit ugly, but it adds a lot of information to the debug log without making calls to the assertion functions more difficult. I believe that I could get rid of the motoRos_complete_ASSERT_XX_XXX functions and just define everything with the macros, but I am not very familiar with the convention there. There's some code duplication that I could try to cut down on too. I haven't tested it very much, I just ran it once and it seemed to work. But is this the sort of thing that you are looking for @gavanderhoorn?

[ted-miller]

Two points:

1. I really struggle to understand some of those naming conventions. My 'knee-jerk' reaction would be to make them more verbose. But they're already fairly long. So we're definitely going to need some form of 'summary block' for internal documentation:

/// <summary>
/// 
/// </summary>
/// <param name="subCodeIfFalse"></param>

2. Let me start by saying that I am not skilled with complex C macros or the 'stringification' (awesome word, btw). But I don't think this is the way to go. AFAICT, there is a one-to-one passthrough of the macro command with the function definition. If that's true, then the macros aren't actually saving you anything. It's just another layer.

I believe that either the rcl or rmw layers offer some examples of some really gnarly macros that basically implement template-classes. I'll see if I can find some stuff and we can discuss together.

[jimmy-mcelwain]

What the macros offer is the ability to read in the variable/expression names that are being tested without having to manually enter that information in as a string. E.g. if I call

motoRos_ASSERT_GE_INT_MESSAGE(g_nodeConfigSettings.ros_domain_id, 
    MIN_ROS_DOMAIN_ID_LINUX,
    SUBCODE_CONFIGURATION_INVALID_DOMAIN_ID,
    "Domain ID (%d) invalid",
    g_nodeConfigSettings.ros_domain_id);

If it fails, then as part of the print out, it will say

Actual: g_nodeConfigSettings.ros_domain_id == -1
Expected: g_nodeConfigSettings.ros_domain_id >= 0 (MIN_ROS_DOMAIN_ID_LINUX)

whereas the non-macro version would say something along the lines of

Actual: -1
Expected: value greater than or equal to 0

I do agree though that my current approach isn't ideal. I could get rid of the stringification altogether, but that would mean that the printout would be less detailed, as shown above. I could instead get rid of the "original" functions and just define it all as a macro, I just didn't do that because I already had the functions written when I decided to try the stringification.

[ted-miller]

Ah. I definitely missed that fact

[jimmy-mcelwain]

I did change the names to be more verbose, and changed some other naming conventions. If some of the names are too long, I can change them back