nrkno / sofie-mos-connection

Sofie MOS Connection: A Part of the Sofie TV Studio Automation System
https://github.com/nrkno/Sofie-TV-automation/
MIT License
22 stars 14 forks source link

Feature Request: Support for iNews heartbeat message #94

Closed PascalViauRC closed 2 months ago

PascalViauRC commented 9 months ago

About me

This RFC is posted on behalf of CBC/Radio-Canada

Use case

When testing with an iNews MOS Gateway, the MOS connection works but there are heartbeat errors in the logs :

{"message":"primary: MosSocketClient: Unknown message: <mos><mosID>SOFIE</mosID><ncsID>SRC</ncsID><heartbeat><time>2024-01-26T12:52:03</time></heartbeat></mos>","level":"error"}
{"message":"Heartbeat on lower: Error: Sent command timed out after 10001 ms","level":"debug"}

In the MOS GATEWAY settings in the UI, the status displays "Bad - Primary: No heartbeats on upper".

It seems that the heartbeat reply from iNews (<mos><mosID>SOFIE</mosID><ncsID>SRC</ncsID><heartbeat><time>2024-01-26T12:52:03</time></heartbeat></mos>) does not have a messageId as required by mos-connection.

Proposal

Add support for the iNews heartbeat message.

Process

The Sofie Team will evaluate this RFC and open up a discussion about it, usually within a week.

jstarpl commented 9 months ago

Hello!

Thank you for contributing to the Sofie Project!

If you haven’t already, please give our contribution guidelines a read.

Do you think you'll be able to try and fix it yourself, or are you posting this as a bug report? The NRK team has no objections to supporting the heartbeat messages without messageId, since we've already accepted a number of PRs to make mos-connection less strict about protocol compliance, but we can't commit to fixing this ourselves.

jstarpl commented 9 months ago

I suppose the handling for these messageId-less heartbeats would go somewhere here: https://github.com/nrkno/sofie-mos-connection/blob/b9a544a88d1eae13a0f220b282d61e5d9859f5da/packages/connector/src/connection/mosSocketClient.ts#L393-L438

Since the entire library is based on the assumption that all MOS messages have a non-empty messageId to handle request-response pairing, this would I think require some sort of a workaround specifically for these heartbeats and possibly looking at this._queueMessages and finding the oldest heartbeat in the queue?

@nytamin do you have any pointers to give here? Should this behavior be an opt-in via some sort of setting in MosConnection, and then later in mos-gateway?

PascalViauRC commented 9 months ago

I opened the RFC at the suggestion of @nytamin, he maybe wanted to integrate it with the new "n-strict" mode. But I could certainly help with a PR.

nytamin commented 9 months ago

Yes, this behaviour should only be present in the non-strict mode, since the messageID is required in the mos-protocol.

When implementing this, be sure to add unit tests that sends a heartbeat reply without a messageID (add them in packages/connector/src/__tests__/Profile0-non-strict.spec.ts, see example in profile0.spec.ts).

nytamin commented 9 months ago

We've had a little chat in the Sofie Team about this now. As this does only affect iNews NRCS's, we won't be looking into implementing a fix for this ourselves, but we'd be happy to accept a PR with a fix.

PascalViauRC commented 9 months ago

All right, I will take a look at it, thanks !

PascalViauRC commented 8 months ago

I’ve been looking at the MOS Protocol and ultimately, the problem is the protocol version used by the iNews Gateway. It is using v2.8, which explains why :

Knowing that, I suggest to reframe this RFC. It should be about adding support for MOS protocol v2.8. I also wonder if we can still consider it as a non-strict feature, since it would use a "valid" (if not recent) MOS protocol version. @jstarpl @nytamin What do you think ?

nytamin commented 8 months ago

Oh, that's a good find @PascalViauRC !

Currently the library only supports the latest version (2.8.5) and since this library can be used to verify mos implementations on other systems I don't think that we should add exceptions to that rule in the strict mode. (non-strict should still be fine though.)

I do see the benefit of being able to support multiple mos versions, especially with the 3.8.4 and 4.0 versions that are starting to gain traction. This would need to be implemented carefully though, and where it should be a configuration property on the mosDevice.

I think that the best course of action would be to continue aiming att supporting the messageId-less heartbeat in non-strict mode (since that would require less effort), and then open a new Feature Request aiming att supporting multiple versions.

PascalViauRC commented 8 months ago

Good idea. When this one is done, I will open a new feature request. Thanks !