Add additional enhancements available on at least some panels/firmware - #15
Add additional enhancements available on at least some panels/firmware#15peternewman wants to merge 29 commits into
Conversation
…onnection status too
…anything as they will be stale
maxajbarlow
left a comment
There was a problem hiding this comment.
Thorough enhancement PR. Covers a lot of ground - new variables, feedbacks, actions, and presets. Here's a detailed review:
What's good:
-
Variable expansion support in IP config (
useVariables: true+context.parseVariablesInString()) - This is a great addition. Users can now template IP addresses from other variables, which is useful in multi-panel setups. -
Regex.IPusage - Replacing inline regex strings with the built-inRegex.IPconstant from@companion-module/baseis the right move. Cleaner and consistent with other modules. -
Expansion1 interface support - Extending network status/link queries to include Expansion1 shows real-world testing on hardware that has this interface.
-
Identify actions/feedbacks/presets - Complete implementation: enable/disable/toggle/fetch + feedback + 4 presets. Follows the established pattern for other toggle features (NMOS, Control Panel).
-
NetworkStatusChangedhandler - Reactive data refresh when the panel reports network changes. Good for keeping variables current without polling. -
Network settings invalidation (
this.networkSettings = nullafter update) - Smart fix. Prevents stale cached settings from being used after an update. -
Artist connection status - Useful for intercom integration monitoring.
Issues to address:
1. typescript-eslint in dependencies (not devDependencies) - Same as noted in #17. This is a dev/lint tool and shouldn't ship as a runtime dependency. Should be moved to devDependencies.
2. Commented-out code in MediaPortAssignment handler:
// this.interfaceLinkStatuses.set(interfaceId, linkStatus)
// this.checkFeedbacks('interfaceLinkStatus')These look like copy-paste artifacts. Should be removed rather than left commented.
3. enableIdentify / disableIdentify use setTimeout for status refresh:
public enableIdentify(): void {
this.sendMessage('/Identify/Enable', {})
setTimeout(() => this.fetchIdentifyStatus(), 500)
}There's also a StatusChanged handler (/Identify/StatusChanged) that calls fetchIdentifyStatus(). So the setTimeout is redundant - the panel will notify via WebSocket when the state changes. The timeout creates a race condition where the polled fetch could arrive before the actual state change. Consider removing the setTimeout and relying solely on the StatusChanged notification, or at minimum documenting why both are needed (e.g., if some firmware versions don't send StatusChanged).
4. Full JSON debug logging of every message:
this.log('debug', `Received: ` + JSON.stringify(data))Same concern as in #14 - this will dump the full body of every WebS message. On panels that send frequent updates (network status, PTP, etc.), this could flood logs. Consider either:
- Removing it (the topic-level log is usually sufficient)
- Only logging the body for unhandled topics (you already log those at
infolevel)
5. artistConnectionStatus feedbacks referenced but not defined:
this.checkFeedbacks('artistConnectionStatus', 'artistConnectionStatusDisplay')These feedback IDs are checked in main.ts but I don't see them defined in the feedbacks.ts diff. If they're not yet implemented, this will silently do nothing (which is fine), but it suggests incomplete feature work. Either add the feedback definitions or remove the checkFeedbacks call.
6. artist_connection_status variable default missing:
The variable definition and getDefaultVariableValues for artist_connection_status is present in variables.ts, but I don't see artist_name being exposed as a variable even though fetchIntercomArtistName() is called. Is the artist name response handler missing from main.ts?
Nits (non-blocking):
- The README variable table update is thorough and matches the code. Well done.
- Preset colour constants (GREEN, RED, BLUE, etc.) - good reuse of existing constants.
NetworkSettingsinterface extension with optional fields (dnsServer1?,dnsServer2?,linkOperationalState?) is forward-compatible.
Summary:
This is a substantial and well-structured enhancement. The core functionality (link status, identify, media port assignment, Artist integration, variable support in IP config) all follows established patterns. The issues above are mostly minor - the setTimeout vs StatusChanged question (#3) and the missing Artist feedback/variable handling (#5/#6) are the most worth addressing. Happy to approve once those are clarified.
…evant options if it's enabled
Fixed.
Still hoping to add these at some point when I get time, but maybe in another PR before this one gets too huge.
Over to you there @maxajbarlow . I just copied the existing patterns and then extended them when I found out about the Changed notifications. Can we remove them or are there
Still very useful generally while the module is still relatively new as any end-user can send a log with useful data in it, rather than it just failing on some mis-match and not knowing what's changed because the topic matched.
These feedbacks have now been added.
Artist name support now added as a variable since I've had a working connection to read it from.
Are you happy to approve and merge now most of this has been covered off @maxajbarlow ? Thanks. P.S. also used it in anger to bulk-set a pile of panels which was useful! |
# Conflicts: # companion/manifest.json # src/config.ts # src/main.ts
|
Heads-up: I merged current Resolution approach — your Bonjour design was favored throughout:
Since #21 effectively duplicated the Bonjour feature this PR already had, #18 was closed and the multi-interface nuance you raised is tracked in #22. Verified locally: |
Thanks for the update @maxajbarlow . What (if anything) do I need to do to this PR to get it merged, and hence avoid further conflicts/drift arising? |
Uh oh!
There was an error while loading. Please reload this page.