Temp to dev #1371

andrew-welker · 2025-12-23T17:13:11Z

This pull request makes several improvements to the BridgeBase.cs and Extensions.cs files in the PepperDash.Essentials.Core project. The main focus is on refactoring and modernizing logging practices, improving code clarity and documentation, and adding support for UDP EISC clients in the bridge factory. The changes also include minor code cleanups and improved error handling.

Key changes include:

Logging Improvements and Refactoring:

Replaced all usages of Debug.LogMessage with strongly-typed extension logging methods such as this.LogDebug, this.LogWarning, this.LogVerbose, this.LogInformation, and this.LogError for more consistent and structured logging throughout BridgeBase.cs. [1] [2] [3] [4] [5] [6] [7]
Improved exception logging to include stack traces and more descriptive error messages in critical methods such as ExecuteJoinAction and Eisc_SigChange.

Code and Documentation Enhancements:

Added and improved XML documentation comments for constructors, methods, and parameters to clarify the purpose and usage of classes and methods, especially in EiscApiAdvanced and EiscApiAdvancedFactory. [1] [2] [3]
Marked the BridgeApi base class as [Obsolete] to indicate its planned removal in a future major version.
Refactored some logic for clarity, such as using pattern matching and simplifying null checks. [1] [2]

Feature Additions:

Added support for UDP EISC client types (eiscapiadvudp, eiscapiadvancedudp) in the EiscApiAdvancedFactory to allow creation of EISC clients over UDP.

General Code Cleanup:

Removed unused usings and commented code, and improved formatting and consistency in both files. [1] [2]
Fixed minor issues such as missing or incorrect parameter documentation. [1] [2]

Minor Improvements in Routing Extensions:

Improved debug log messages for routing to include both source and destination keys and signal type for better traceability.
Improved exception handling formatting in the routing extension methods.

These changes collectively improve maintainability, observability, and clarity of the bridge and routing infrastructure.

Logging improvements and refactoring:
- Replaced Debug.LogMessage with strongly-typed logging extension methods across bridge classes for better log structure and consistency. [1] [2] [3] [4] [5] [6] [7]
- Enhanced exception logging to include stack traces and more descriptive error messages.
Code and documentation enhancements:
- Added and improved XML documentation for constructors, parameters, and methods, and marked BridgeApi as obsolete. [1] [2] [3] [4]
Feature additions:
- Added UDP EISC client support in EiscApiAdvancedFactory with new type names.
General code cleanup:
- Removed unused usings, improved formatting, and clarified parameter documentation. [1] [2] [3] [4]
Routing extensions improvements:
- Improved debug log messages for routing and exception handling. [1] [2]

- fixed multiple enumeration exception

generic sink

Copilot

Pull request overview

This pull request modernizes logging practices across the bridge and routing infrastructure by migrating from Debug.LogMessage to strongly-typed extension logging methods. It also adds UDP EISC client support, improves XML documentation, and refactors several messenger classes to use strongly-typed message objects. The changes focus on improving code maintainability, observability, and consistency.

Key Changes:

Migrated logging from Debug.LogMessage to structured logging extension methods (this.LogDebug, this.LogWarning, etc.) throughout BridgeBase.cs and improved exception logging with stack traces
Added UDP EISC client support in EiscApiAdvancedFactory with new type names eiscapiadvudp and eiscapiadvancedudp
Refactored DeviceVolumeMessenger to use strongly-typed message classes instead of anonymous objects, and implemented ICurrentSources interface in GenericSink

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
DeviceVolumeMessenger.cs	Refactored to use strongly-typed `VolumeStateMessage` and `Volume` classes instead of anonymous objects for better type safety
CurrentSourcesMessenger.cs	Added dictionary copying with `ToDictionary` to prevent enumeration issues during event handling
GenericSink.cs	Implemented `ICurrentSources` interface with `SetCurrentSource` method to track current audio/video sources
Extensions.cs	Minor formatting improvements and enhanced log messages to include destination key information
BridgeBase.cs	Comprehensive logging migration to extension methods, improved XML documentation, added UDP EISC support, and fixed bugs in `ExecuteJoinAction` method

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

Copilot · 2025-12-23T17:16:24Z

src/PepperDash.Essentials.Devices.Common/Generic/GenericSink.cs

+            foreach (eRoutingSignalType type in Enum.GetValues(typeof(eRoutingSignalType)))
+            {
+                var flagValue = Convert.ToInt32(type);
+                // Skip if flagValue is 0 or not a power of two (i.e., not a single-bit flag).
+                // (flagValue & (flagValue - 1)) != 0 checks if more than one bit is set.
+                if (flagValue == 0 || (flagValue & (flagValue - 1)) != 0)
+                {
+                    this.LogDebug("Skipping {type}", type);
+                    continue;
+                }
+
+                this.LogDebug("setting {type}", type);
+
+                if (signalType.HasFlag(type))
+                {
+                    UpdateCurrentSources(type, sourceListKey, sourceListItem);
+                }
+            }


The condition (flagValue & (flagValue - 1)) != 0 correctly identifies composite flags, but the log message says "Skipping" when these should be skipped. However, the real issue is that this enumeration approach is inefficient. The code iterates through all enum values including composite flags just to filter them out. Consider using only the specific flags you need (e.g., Audio, Video) rather than enumerating all possible enum values.

Copilot · 2025-12-23T17:16:24Z

src/PepperDash.Essentials.Devices.Common/Generic/GenericSink.cs

+                    this.LogDebug("Skipping {type}", type);
+                    continue;
+                }
+
+                this.LogDebug("setting {type}", type);
+
+                if (signalType.HasFlag(type))
+                {


The debug log statements at lines 62 and 66 will be executed for every enum value checked, which could generate excessive log noise. These debug logs should either be removed or only logged when meaningful work is being done (i.e., when actually updating sources).

Suggested change

this.LogDebug("Skipping {type}", type);

continue;

}

this.LogDebug("setting {type}", type);

if (signalType.HasFlag(type))

{

continue;

}

if (signalType.HasFlag(type))

{

this.LogDebug("setting {type}", type);

Copilot · 2025-12-23T17:16:24Z

src/PepperDash.Essentials.Core/Bridges/BridgeBase.cs

+                        eisc = new EthernetIntersystemCommunications(controlProperties.IpIdInt,
+                            controlProperties.TcpSshProperties.Address, Global.ControlSystem);


The UDP EISC client creation uses EthernetIntersystemCommunications constructor without specifying the transport type. This constructor signature typically creates a TCP client by default. For UDP support, you likely need to use a different constructor overload or specify the transport protocol explicitly to ensure UDP is actually used instead of TCP.

Suggested change

eisc = new EthernetIntersystemCommunications(controlProperties.IpIdInt,

controlProperties.TcpSshProperties.Address, Global.ControlSystem);

eisc = new EthernetIntersystemCommunications(

controlProperties.IpIdInt,

controlProperties.TcpSshProperties.Address,

eDeviceEthernetProtocols.UDP,

Global.ControlSystem);

Copilot · 2025-12-23T17:16:25Z

src/PepperDash.Essentials.Core/Routing/Extensions.cs

            // otherwise, audioVideo needs to be handled as two steps.

-            Debug.LogMessage(LogEventLevel.Debug, "Attempting to build source route from {sourceKey} of type {type}", destination, source.Key);
+            Debug.LogMessage(LogEventLevel.Debug, "Attempting to build source route from {destinationKey} to {sourceKey} of type {type}", destination, source.Key, signalType);


The log message is missing a parameter placeholder. The message format includes '{destinationKey}' but the Debug.LogMessage call only provides two arguments after the message: destination (the object) and source.Key. The destination key should be passed as destination.Key to match the placeholder.

Suggested change

Debug.LogMessage(LogEventLevel.Debug, "Attempting to build source route from {destinationKey} to {sourceKey} of type {type}", destination, source.Key, signalType);

Debug.LogMessage(LogEventLevel.Debug, "Attempting to build source route from {destinationKey} to {sourceKey} of type {type}", destination, destination.Key, source.Key, signalType);

Copilot · 2025-12-23T17:16:25Z

src/PepperDash.Essentials.MobileControl.Messengers/Messengers/CurrentSourcesMessenger.cs

+        // need to copy the dictionaries to avoid enumeration issues
+        var currentSourceKeys = sourceDevice.CurrentSourceKeys.ToDictionary(kvp => kvp.Key, kvp => kvp.Value);
+        var currentSources = sourceDevice.CurrentSources.ToDictionary(kvp => kvp.Key, kvp => kvp.Value);


The comment says "need to copy the dictionaries to avoid enumeration issues" but the code uses ToDictionary which creates a shallow copy. If the dictionary values themselves are mutable objects, this won't prevent modification issues during enumeration. Consider whether a deep copy is needed, or if locking the dictionaries during access would be more appropriate.

Copilot · 2025-12-23T17:16:25Z

src/PepperDash.Essentials.MobileControl.Messengers/Messengers/DeviceVolumeMessenger.cs

+                if (device is IBasicVolumeWithFeedbackAdvanced volumeAdvanced)
+                {
+                    message.Volume.RawValue = volumeAdvanced.RawVolumeLevel.ToString();
+                    message.Volume.Units = volumeAdvanced.Units;
+                }


The variable check here accesses device instead of feedback. The event handler receives feedback as a parameter and should check if feedback implements IBasicVolumeWithFeedbackAdvanced, not the outer device variable. This could result in incorrect behavior if the feedback object differs from the device.

andrew-welker and others added 11 commits December 5, 2025 16:26

fix: GenericSink implements ICurrentSources

2048e3f

fix: initialize current sources dictionaries

6a33e7c

fix: copy dictionaries

bce1e36

- fixed multiple enumeration exception

fix: implement PR Review suggestions

210b398

Merge pull request #1368 from PepperDash/generic-sink

a7b88ec

generic sink

fix: DeviceVolumeMessenger only sends rawValue when device implements it

c5b0872

Merge pull request #1369 from PepperDash/volume-messenger-issue

7c8f058

fix: support for legacy UDP EISC

092896b

chore: update comments, debug methods, and mark some things obsolete

076e5df

fix: ExecuteJoinAction now uses the correct Sig collections

2cc37a4

Merge pull request #1370 from PepperDash/udp-eisc

39c1f60

andrew-welker requested review from Copilot and ndorin December 23, 2025 17:13

Copilot started reviewing on behalf of andrew-welker December 23, 2025 17:13 View session

ndorin approved these changes Dec 23, 2025

View reviewed changes

ndorin merged commit ce8b08e into development Dec 23, 2025
9 checks passed

ndorin deleted the temp-to-dev branch December 23, 2025 17:14

Copilot AI reviewed Dec 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Temp to dev #1371

Temp to dev #1371

Uh oh!

andrew-welker commented Dec 23, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 23, 2025

Uh oh!

Copilot AI Dec 23, 2025

Uh oh!

Copilot AI Dec 23, 2025

Uh oh!

Copilot AI Dec 23, 2025

Uh oh!

Copilot AI Dec 23, 2025

Uh oh!

Copilot AI Dec 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		eisc = new EthernetIntersystemCommunications(controlProperties.IpIdInt,
		controlProperties.TcpSshProperties.Address, Global.ControlSystem);

	Debug.LogMessage(LogEventLevel.Debug, "Attempting to build source route from {destinationKey} to {sourceKey} of type {type}", destination, source.Key, signalType);
	Debug.LogMessage(LogEventLevel.Debug, "Attempting to build source route from {destinationKey} to {sourceKey} of type {type}", destination, destination.Key, source.Key, signalType);

Temp to dev #1371

Temp to dev #1371

Uh oh!

Conversation

andrew-welker commented Dec 23, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants