feat: add circuitType property to IOPortConfig #1377
Conversation
- adds a config value that mutes the display when the screen is in the up position - screens will now mute/unmute based on their position if the config is set
Enhanced error and warning message generation to use monitor names when available, include counts, proper pluralization, and append "Offline" when issues are present. Avoided multiple enumerations by converting to lists. "Room Ok." is shown when no issues are detected.
…version in digital input devices
There was a problem hiding this comment.
Pull request overview
This pull request adds support for configuring the circuit type (Normally Open "NO" or Normally Closed "NC") for digital input devices, enabling automatic state inversion for NC circuits. This enhances flexibility when integrating different hardware configurations.
Changes:
- Added a
CircuitTypeproperty toIOPortConfigclass with a default value of "NO" - Updated
GenericDigitalInputDeviceto invert input state when CircuitType is set to "NC" - Updated
GenericVersiportDigitalInputDeviceto invert digital input state when CircuitType is set to "NC"
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/PepperDash.Essentials.Core/CrestronIO/IOPortConfig.cs | Added CircuitType property with JSON serialization to support NO/NC configuration |
| src/PepperDash.Essentials.Core/CrestronIO/GenericDigitalInputDevice.cs | Added invertState field and logic to conditionally invert input state based on circuit type |
| src/PepperDash.Essentials.Core/CrestronIO/GenericVersiportInputDevice.cs | Added invertState field and logic to conditionally invert input state based on circuit type |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| config.CircuitType.Equals("NC", StringComparison.OrdinalIgnoreCase); | ||
|
|
||
| InputStateFeedback = new BoolFeedback("inputState", () => invertState ? !inputPort.DigitalIn : inputPort.DigitalIn); | ||
| PartitionPresentFeedback = new BoolFeedback("partitionPresent", () => !inputPort.DigitalIn); |
There was a problem hiding this comment.
The PartitionPresentFeedback does not respect the CircuitType configuration. While InputStateFeedback correctly inverts based on the invertState flag, PartitionPresentFeedback always uses !inputPort.DigitalIn without considering the circuit type. For consistency, if the circuit is NC (normally closed), the partition present logic should also be adjusted. Consider whether PartitionPresent should use the inverted input state or if it has different semantics that justify the current implementation.
| /// If set to "NC", the input state will be inverted. Defaults to "NO" if not specified. | ||
| /// </summary> | ||
| [JsonProperty("circuitType")] | ||
| public string CircuitType { get; set; } = "NO"; |
There was a problem hiding this comment.
The CircuitType property allows any string value but only "NO" and "NC" are meaningful. Consider using an enum instead of a string to enforce valid values at compile time, or add validation to ensure only valid values are accepted. This would prevent configuration errors from typos like "no" (lowercase) or "NORMALLY_OPEN".
| invertState = !string.IsNullOrEmpty(config.CircuitType) && | ||
| config.CircuitType.Equals("NC", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
The null/empty check for CircuitType is unnecessary since the property has a default value of "NO" (line 46 in IOPortConfig.cs). The check !string.IsNullOrEmpty(config.CircuitType) will always be true unless the configuration explicitly sets it to null or empty string, which would be a configuration error. Consider removing this check or handling null/empty as equivalent to "NO".
| invertState = !string.IsNullOrEmpty(config.CircuitType) && | |
| config.CircuitType.Equals("NC", StringComparison.OrdinalIgnoreCase); | |
| invertState = string.Equals(config.CircuitType, "NC", StringComparison.OrdinalIgnoreCase); |
| invertState = !string.IsNullOrEmpty(config.CircuitType) && | ||
| config.CircuitType.Equals("NC", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
The null/empty check for CircuitType is unnecessary since the property has a default value of "NO" (line 46 in IOPortConfig.cs). The check !string.IsNullOrEmpty(config.CircuitType) will always be true unless the configuration explicitly sets it to null or empty string, which would be a configuration error. Consider removing this check or handling null/empty as equivalent to "NO".
| invertState = !string.IsNullOrEmpty(config.CircuitType) && | |
| config.CircuitType.Equals("NC", StringComparison.OrdinalIgnoreCase); | |
| var circuitType = string.IsNullOrEmpty(config.CircuitType) ? "NO" : config.CircuitType; | |
| invertState = circuitType.Equals("NC", StringComparison.OrdinalIgnoreCase); |
| public class GenericDigitalInputDevice : EssentialsBridgeableDevice, IDigitalInput, IHasFeedback | ||
| { | ||
| private DigitalInput inputPort; | ||
| private bool invertState; |
There was a problem hiding this comment.
Field 'invertState' can be 'readonly'.
| private bool invertState; | |
| private readonly bool invertState; |
| public class GenericVersiportDigitalInputDevice : EssentialsBridgeableDevice, IDigitalInput, IPartitionStateProvider, IHasFeedback | ||
| { | ||
| private Versiport inputPort; | ||
| private bool invertState; |
There was a problem hiding this comment.
Field 'invertState' can be 'readonly'.
| private bool invertState; | |
| private readonly bool invertState; |
…c' into feature/circuittype-property-versiport
This pull request adds support for configuring the input circuit type (normally open "NO" or normally closed "NC") for digital input devices, allowing the input state to be automatically inverted if the circuit is set to "NC". This improves flexibility when integrating different hardware configurations.
Digital Input State Inversion Support:
CircuitTypeproperty to theIOPortConfigclass, allowing configuration of input circuit type ("NO" or "NC"). Defaults to "NO" if not specified.GenericDigitalInputDeviceto use theCircuitTypeproperty: if set to "NC", the input state is inverted when reporting feedback. [1] [2]GenericVersiportDigitalInputDeviceto use theCircuitTypeproperty: if set to "NC", the digital input state is inverted when reporting feedback. [1] [2]…version in digital input devices