Feature client info

[dmweir]

Main changes:

• hello/goodbye/introduce messages from MM on client connect/disconnect
• Support for ConnectV2 with module name and pid
• Expanded module id range (module_id is no longer used as array index)
• Refactor ProcessMessage to use handle methods
• Store a reusable CMessage field in MM class
• Heap allocate MessageManager instance
• Removed TimingMessage
  - Still need to decide if we want to do something here.
  - Is sending the packets themselves is any less efficient than sending the giant TimingMessage?

It should also eliminate any application level pinging code. Now any module that cares about the status of other modules could subscribe to HELLO/GOODBYE. Sending an INTRODUCE signal on module startup will trigger message manager to direct send a HELLO msg for each connected module.

[Copilot]

Pull request overview

This PR extends MessageManager to broadcast client lifecycle signals (HELLO/GOODBYE/INTRODUCE), adds ConnectV2 client metadata (name/pid/address/port), and refactors MM message handling while removing legacy timing/internal timer functionality from the core RTMA API.

Changes:

• Add HELLO/GOODBYE/INTRODUCE signals and ConnectV2 metadata propagation from MessageManager.
• Refactor MessageManager routing/subscription logic to use record pointers/UIDs instead of module-id-as-array-index.
• Remove RTMA internal timers / timing message plumbing and disable timer entrypoints in the Matlab binding.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
src/modules/MessageManager/MessageManager.vcxproj	Switch MM build output/subsystem settings (Console, explicit output path).
src/modules/MessageManager/MessageManager.h	Expand module record (uid/pid/name/addr/port), change subscriber tracking to UID, add new MM handler APIs.
src/modules/MessageManager/MessageManager.cpp	Refactor ProcessMessage into handler methods; add HELLO/GOODBYE/INTRODUCE behavior; change routing to use module records/UIDs.
src/core/RTMA.cpp	Remove internal timer thread implementation and associated cleanup.
src/core/PipeLib/SocketPipe.h	Add ws2tcpip include and declare GetIpAddress.
src/core/PipeLib/SocketPipe.cpp	Implement GetIpAddress on SocketPipe.
lang/matlab/MatlabRTMA.cpp	Disable SET_TIMER / CANCEL_TIMER opcodes (always fail).
include/RTMA.h	Remove RTMA_Module timer APIs from the public interface.
include/RTMA_types.h	Expand module-id range model; add new client-info message defs; remove many legacy defs.
include/internal/UPipe.h	Add GetIpAddress to the UPipe interface.

Comments suppressed due to low confidence (3)

src/modules/MessageManager/MessageManager.cpp:290

• mod_id is not defined in this scope, which will fail to compile. This log line should reference the subscriber module record being forwarded to.

				DEBUG_TEXT_("Forwarding message to module " << mod_id << "... ");

src/modules/MessageManager/MessageManager.cpp:861

• mod_id is not defined in this scope, which will fail to compile. Use the destination module's ModuleID for logging.

		DEBUG_TEXT_("Sending ACK to module " << mod_id << "... ");

src/modules/MessageManager/MessageManager.h:104

• SetGoodbye does not zero-initialize the outgoing struct and uses full-buffer memcpy, which can leak uninitialized bytes to subscribers. Prefer zeroing and bounded string copy with explicit NUL-termination.

	void SetGoodbye(MDF_GOODBYE *goodbye) {
		goodbye->uid = uid;
		goodbye->mod_id = ModuleID;
		goodbye->pid = pid;
		goodbye->port = port;

		if (addr != NULL) {
			memcpy(goodbye->addr, addr, sizeof(goodbye->addr));
		}
		else {
			goodbye->addr[0] = '\0';
		}

		if (name != NULL) {
			memcpy(goodbye->name, name, sizeof(goodbye->name));
		}
		else {
			goodbye->name[0] = '\0';
		}
	}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

🟡 Not ready to approve

Several introduced code paths have clear compile/runtime defects (e.g., CModuleRecord::Reset recursion, undefined variables, and Unix build break in SocketPipe::GetIpAddress).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (4)

src/modules/MessageManager/MessageManager.cpp:863

• This debug log references mod_id, but SendAcknowledge now receives a CModuleRecord* and has no mod_id local. This will fail to compile; use mod->ModuleID.

		DEBUG_TEXT_("Sending ACK to module " << mod_id << "... ");

src/core/PipeLib/SocketPipe.cpp:468

• SocketPipe::GetIpAddress checks getpeername(...) == SOCKET_ERROR, but SOCKET_ERROR is a Winsock constant and is not defined on Unix builds. This causes a compile error on non-Windows; use a platform-specific error check.

	if (getpeername(_hPipe.id, (struct sockaddr*)&peer, &namelen) == SOCKET_ERROR) {
		addr[0] = '\0';
		*port = 0;
		return 0;
	}

src/modules/MessageManager/MessageManager.h:367

• GetDynamicModuleId() no longer checks whether curr_id is already in use before returning it. If any module connects with an explicit module ID within the dynamic range, the allocator can later hand out the same ID again, creating duplicate ModuleID values and breaking routing.

	MODULE_ID GetDynamicModuleId()
	{
		MODULE_ID curr_id = m_NextDynamicModId;

		if (curr_id > MAX_MODULE_ID) {

src/modules/MessageManager/MessageManager.cpp:31

• main heap-allocates CMessageManager but never deletes it on normal exit. Even though the process may run for a long time, this still leaks on early return paths and complicates leak checking/tools.

		CMessageManager* MM = new CMessageManager();
		MM->MainLoop(options);
		return 0;

• Files reviewed: 19/19 changed files
• Comments generated: 3
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

It introduces concrete security issues (uninitialized/over-copied data in GOODBYE/name handling) and breaks build/API compatibility (new uint16_t usage without guaranteed definition and removed timer APIs still referenced by bindings).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (6)

src/modules/MessageManager/MessageManager.cpp:560

• ConnectModuleV2 reserves a CModuleRecord via GetOpenRecord() before validating that a usable module_id is available. If dynamic ID allocation fails (returns 0), the reserved slot remains in-use (uid stays set) and reduces capacity permanently. Release/reset the record when module_id <= 0.

			if (module_id > 0)
			{
				DEBUG_TEXT("Connecting module " << module_id << " on pipe " << pSourcePipe);

				mod->ModuleID = module_id;

include/internal/UPipe.h:154

• UPipe now exposes GetIpAddress(char*, uint16_t*, ...) but this header doesn't include a definition for uint16_t. This can break compilation depending on include order/toolchain; ensure the type is defined in UPipe.h (e.g., include <stdint.h>/<cstdint>), and keep derived-class overrides consistent.

	// never blocks (if no space in pipe, then returns with 0). Returns number of
	// bytes written.
	virtual int Write( void *data_buffer, int n_bytes, double timeout) = 0;
	virtual int GetIpAddress(char* addr, uint16_t* port, int bufsz) = 0;
};

lang/matlab/MatlabRTMA.cpp:258

• SET_TIMER now silently returns -1 even when connected, which can mask a breaking API change for Matlab callers. Prefer failing loudly (e.g., Error(...)) or removing the command to avoid false-success behavior in existing scripts.

        case SET_TIMER:

            if( num_input_args < 2) Error( "incorrect number of arguments");
            TimerID = -1;
            output_arg[0] = mxCreateDoubleScalar( (double) TimerID);
            break;

lang/matlab/MatlabRTMA.cpp:264

• CANCEL_TIMER now silently returns -1, which can look like an operational failure rather than an unsupported feature. Prefer surfacing an explicit error to guide callers to the replacement mechanism (or remove the command entirely).

        case CANCEL_TIMER:
            if( num_input_args < 2) Error( "incorrect number of arguments");
            status = -1;
            output_arg[0] = mxCreateDoubleScalar( (double) status);
            break;

include/RTMA_types.h:93

• This header drops several core module ID constants (MID_COMMAND_MODULE, MID_APPLICATION_MODULE, MID_NETWORK_RELAY, MID_STATUS_MODULE), but they are still referenced by checked-in bindings (e.g., lang/python/PyRTMA3.py:362+ and lang/python/RTMA_Definitions3.py:151+). Reintroduce these defines for backward compatibility or update/regenerate the bindings in the same PR.

// Module ID-s of core modules
#define MID_MESSAGE_MANAGER     0
#define MID_QUICKLOGGER         5

src/modules/MessageManager/MessageManager.cpp:529

• If module_id == 0 and GetDynamicModuleId() fails (returns 0), this function keeps the CModuleRecord slot reserved (uid stays set) but never finalizes the record, effectively leaking a connection slot. Ensure the reserved record is released on failure paths.

This issue also appears on line 556 of the same file.

			if (module_id > 0)
			{
				DEBUG_TEXT("Connecting module " << module_id << " on pipe " << pSourcePipe);

				// Create a module record

• Files reviewed: 20/20 changed files
• Comments generated: 3
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[dmweir]

@jmw182 I ended up running copilot a few times and ran some other checks locally to clean up old bugs in the rtma code. Maybe if there are others things that you want to remove or cleanup, you can add it to this branch.

[jmw182]

I'll try to review next week. I worked on #7 a couple of years ago so I can see if there's anything from there that makes sense to pull into this.

[jmw182]

@copilot compare this PR with #7

[Copilot]

@copilot compare this PR with #7

Compared PR #10 against PR #7.

• Scope: PR Feature client info #10 is much narrower (23 files, +1702/-2754) vs PR Cleanup and add compatibility with pyrtma v2.3 #7 (94 files, +2385/-5488).
• Overlap: 21 files overlap, mainly MessageManager, RTMA.h, RTMA_types.h, and PipeLib internals.
• Common intent: both touch CONNECT_V2 support and core type/header compatibility.
• Main difference: PR Cleanup and add compatibility with pyrtma v2.3 #7 also included broad binding/tooling cleanup (notably .NET/Matlab wrapper updates), while PR Feature client info #10 focuses on client lifecycle signaling (HELLO/GOODBYE/INTRODUCE), dynamic module-ID handling, and timer-path removal in core/MM.

If you want to pull ideas from #7 into this branch, the highest-value candidates are the wrapper compatibility updates (especially lang/dot_net/RTMA.NET.h and related Matlab-side API handling) since those are where current compatibility concerns overlap most.

[dmweir]

@jmw182 I somewhat arbitrarily selected the expanded ranges for dynamic and static module ids as well as the max modules if you other ideas on what those should be. We should be free to change these values now that the module_id is no longer the index into the module record array.

[jmw182]

@jmw182 I somewhat arbitrarily selected the expanded ranges for dynamic and static module ids as well as the max modules if you other ideas on what those should be. We should be free to change these values now that the module_id is no longer the index into the module record array.

@dmweir we will need to make appropriate changes in pyrtma for this to work. pyrtma, and loader in particular, is incompatible with this build of message manager because it throws InvalidDestinationModule errors when attempting to send a message or signal to a specific destination module with ID > MAX_MODULES. This can be updated to use MAX_MODULE_ID after updating pyrtma.core_defs to match the latest rtma types.

[dmweir]

I think it mostly complete, but should double check: https://github.com/pitt-rnel/pyrtma/tree/feature-client_info

[jmw182]

I pushed a fix to the MAX_MODULE_ID check I mentioned to that branch (pitt-rnel/pyrtma#60), and got that branch working with this one.

Conductor will also need some modifications to ModuleManager. It currently depends on TIMING_MESSAGE for pid tracking.

[jmw182]

I made the minimum necessary conductor changes here: https://github.com/CorticalBionics/conductor/pull/10