Add debug broadcast enable/disable option and debug broadcast port option to config file

[jimmy-mcelwain]

I added 2 extra settings to the motoros2_config.yaml file, userlan_debug_broadcast_enabled and userlan_debug_broadcast_port. By default, it broadcasts to all enabled ports, but the user can choose a specific port if they only want it to broadcast to one port, or the user can disable it.

[github-actions]

yamllint Failed

Show Output

::group::config/motoros2_config.yaml
::error file=config/motoros2_config.yaml,line=289,col=44::289:44 [trailing-spaces] trailing spaces
::error file=config/motoros2_config.yaml,line=292,col=80::292:80 [trailing-spaces] trailing spaces
::error file=config/motoros2_config.yaml,line=295,col=80::295:80 [trailing-spaces] trailing spaces
::error file=config/motoros2_config.yaml,line=303,col=73::303:73 [trailing-spaces] trailing spaces
::error file=config/motoros2_config.yaml,line=306,col=80::306:80 [trailing-spaces] trailing spaces
::error file=config/motoros2_config.yaml,line=307,col=79::307:79 [trailing-spaces] trailing spaces
::error file=config/motoros2_config.yaml,line=309,col=67::309:67 [trailing-spaces] trailing spaces
::error file=config/motoros2_config.yaml,line=313,col=81::313:81 [trailing-spaces] trailing spaces
::endgroup::

Workflow: CI: lint MotoROS2 config, Action: __karancode_yamllint-github-action, Lint: config

[jimmy-mcelwain]

I have tested pretty thoroughly on a YRC1000 on humble and I did not run into any issues.

[gavanderhoorn]

CI failures of the msbuild tasks are expected. Any other failures would have to be addressed.

[jimmy-mcelwain]

@gavanderhoorn I went through and fixed the other failing checks, thanks for pointing that out. I wasn't sure which checks I should ignore.

[ted-miller]

Debug broadcasting will be disabled for this session

I don't like this behavior. I definitely think broadcasting should be enabled if there's an error in the configuration.

[jimmy-mcelwain]

I can change that. So userlan_monitoring_enabled should be set to disabled if there is an error in the config, but broadcasting should still be enabled? I only ask because they're in the same case for the config switch case.

[ted-miller]

I don't have an opinion on the behavior of userlan_monitoring_enabled in this case

[gavanderhoorn]

UserLan monitoring is extra functionality: it makes detecting disconnections faster, but isn't absolutely necessary (other places will also detect disconnects, they'll just take longer).

There's no harm in disabling it if the configuration is incorrect. And, we really can't do anything if configuration is incorrect: monitoring only a single port (when two are present) would be incorrect (which one to monitor in that case?) and monitoring all of them would also be incorrect -- it would likely result in MotoROS2 detecting disconnects 'all the time'. So disabling is really the only thing we can do.

But debug broadcasts should indeed not be disabled in cases of incorrect configuration. Especially since we often use debug logs to diagnose misconfiguration.

[jimmy-mcelwain]

I changed the behavior and this documentation so that it broadcasts to all ports if the specific port isn't active.

[gavanderhoorn]

Did you have a particular reason changing the phrasing here?

[jimmy-mcelwain]

There are now 2 different UserLan ports that the user can specify (monitor and debug broadcast), so I added the word "monitor" to clarify which one this condition checked. But then that put it above 32 characters (the alarm message limit), so I chose to shorten "Invalid" to "Bad" to fit in the character limit. I think that "invalid" is a better word for that, but it's more important to be specific about which config setting needs to be changed.

[gavanderhoorn]

So should we instead separate parsing and error reporting for monitoring and debug log port?

Would lead to some mild code duplication, but would also make parsing clearer and avoids this situation.

[jimmy-mcelwain]

I don't believe that this specific message is affected by combined parsing/error reporting for the two port config options. My preference would be to keep the parsing as combined, but I would be fine with changing it.

[gavanderhoorn]

I understand why you opted to reuse the Ros_UserLan_Port_Setting type, but it does make the parsing code a little harder to read.

[jimmy-mcelwain]

Yeah I tried doing it both ways, and this had much less duplicated code and was overall cleaner in my opinion. I don't especially like it either though.

[gavanderhoorn]

what would be the value (ie: string) for 'all available network ports'? Because the documentation states that's the default, but I can't set this to 'all available network ports' I believe.

[jimmy-mcelwain]

So there is not a dedicated string for 'all available ports'. I was planning on having that, but part of the weirdness with it having the same type as userlan_monitor_port (Ros_UserLan_Port_Setting) is that even if they have different options with different behavior (e.g. userlan_monitor_port has 'auto detection' and userlan_debug_broadcast_port has 'all available ports'), they are parsed the same. So I had the behavior match that of userlan_monitor_port, which doesn't have a specific string for 'auto detection'. That being said, unlike userlan_monitor_port, if the user types in an invalid string, it will default to 'all available ports' rather than 'disabled'

[gavanderhoorn]

I'll test this on my YRC1 and FS1 next week.

[gavanderhoorn]

I've not tested this extensively yet (I will try to do that now), but as a high-level initial comment I might suggest adding some more comments.

From a quick read it's not immediately clear why only socket[0] is initialised in Ros_Debug_SetFromConfig() fi. The interaction between Ros_Debug_SetFromConfig() and Ros_Debug_Init() could also use some comments.

[gavanderhoorn]

this might also be a nice place to explain why this will allow control flow to continue here if g_nodeConfigSettings.log_to_stdout == TRUE?

(because we can reuse all the machinery after this to format the message and as ros_debugPorts.enabledPortCount is most likely 0, nothing will be sent out over any of the LAN ports, but it will be printed to the controller's console. IIUC).

[gavanderhoorn]

@jimmy-mcelwain: I'm wondering whether this works as intended (specifically this line).

Could it be this prevents logs from getting broadcast until at least the config file has been parsed? Both debug_broadcast_enabled and log_to_stdout will be FALSE in that case I believe.

Looking at the debug log output of my YRC1, it seems to be missing at least these printouts (maybe more):

motoros2/src/main.c

Lines 60 to 64 in c61fb86

	Ros_Debug_BroadcastMsg("---");
	Ros_Debug_BroadcastMsg(APPLICATION_NAME " %s - boot", APPLICATION_VERSION);
	Ros_Debug_BroadcastMsg("M+ libmicroros version: '%s'", MOTOPLUS_LIBMICROROS_VERSION_STR);
	Ros_Debug_BroadcastMsg("PlatformLib version: %u.%u.%u", MOTOROS_PLATFORM_LIB_MAJOR,
	MOTOROS_PLATFORM_LIB_MINOR, MOTOROS_PLATFORM_LIB_PATCH);

[jimmy-mcelwain]

I think that you're right. Maybe if we call Ros_ConfigFile_SetAllDefaultValues() to set debug_broadcast_enabled to TRUE in main before any calls to Ros_Debug_BroadcastMsg(), then we would be able to broadcast those messages. I would need to think it through more though, that might require more changes.