Comments following review

[andrewbonney]

I'm copying a few comments here that I originally sent via e-mail, just so they're a bit easier to track and resolve. Responses from @cristian-recoseanu are noted underneath in italics.

1. connection_lost - I didn't understand this at first. I’d suggest adding a brief description and/or link to MQTT WILL. My interpretation (having read other docs) is that the connection_lost is sent on startup to tell the broker what to send out to clients if an ungraceful disconnect is detected. This intentionally has no timing data as it isn't known when it will get sent out (I wondered why this was missing at first). Do you also need to specify the topic this will be sent to? This may be the same topic as used for normal messages.

• Your assumption is correct. Should we link to a more detailed explanation of MQTT WILL messages? The topic used is the same across all messages and is defined in the IS-05 parameter “broker_topic”

2. I’d suggest being clear that for WebSocket comms, even though there is one URL, each client gets its own communication channel with unique data. This is probably the standard expectation for a WebSocket, but it threw me slightly at first and I suspect it would throw others too.

• Ok so basically clarify that each connection is independent and considered its own unique sessions with unique subscriptions and events sent?

3. The example event type bool/enum/OnOff seems inconsistent, should it be boolean/enum/OnOff?

• Well spotted. That is indeed a mistake.

4. I’d appreciate recommendations for frequency of access to the API to get bounds of a numeric event type for example. Is this allowed to change over its lifetime without disconnecting all clients - it may be easier if not, then a client knows to re-query this upon reconnection?

• I’m not sure I fully understand this one. Could you give an example?

5. Be clear with wildcards that you can't do stuff like number/Tempera* or number/*/C. I assume the * should replace whole fields only and always goes at the end?

• Understood. The documentation does mention the wildcard always goes at the end.

6. I’d suggest re-wording the core models section slightly to assume that those changes are already part of IS-04/05 and specify the version of 04 and 05 required (v1.3 and v1.1 respectively).

• Would that require we have the feature requests logged on Github for both IS-04 and IS-05? Is your suggestion to still keep the section to highlight the changes but write it as if it had already happened?

7. Remove AMQP from core models diagram

• Again well spotted

8. 'channels' needs removing from the core models Source example as this relates to the 'audio' schema

• Understood

9. Is _nmos-mqtt TCP or UDP? Guessing it should be '_nmos-mqtt._tcp'? Others seem to use _mqtt._tcp, should we be using that too, or is there a reason you'd want a specific NMOS MQTT server?

• It is indeed TCP. I think we wanted to be clear that it is the MQTT broker which is involved in NMOS. I don’t know what the odds are of a customer already having an MQTT broker or not.
  I don’t have a strong opinion on this and we just made a decision so we could move on with the work. Would you recommend using the simple _mqtt._tcp instead?

[andrewbonney]

Responses below...

1. Yes, I'd suggest linking to more detailed documentation. The use of the same topic should probably be explicitly noted in the docs.

2. Yes

3. It seems important for a Receiver to know about the max and min values it may receive, or the possible descriptions for an enum for example. As such, when an IS-05 connection is first made, I'd assume that the Receiver would make a request to the IS-07 API to get the current type definition. My query is whether the Receiver should expect this type definition to change at all over its lifetime, for example adding another value to an enum or changing a max/min value. If these could change over the lifetime, what sort of polling interval should the Receiver use to contact the API? A simpler approach may be to mandate that if the type definition changes, any existing connections must be broken. This would then ensure that a GET to /type would only occur immediately following an IS-05 connection request.

4. We do need to get PRs in place against the 04 and 05 repositories, but I think this is in hand. This is just a suggestion that for this specification, it should indicate which versions of 04 and 05 it will work with (which we should assume to be v1.3 and v1.1 at this point), and the rest of the text should as you suggest assume these are already published, rather than wording this spec document as a request for changes elsewhere.

5. I don't have a strong opinion here, so feel free to leave it as it is (but adding the ._tcp) unless others suggest otherwise.

[andrewbonney]

A few more comments. Sorry for the volume, just want to make sure there's a note of these before the deadline:

10. The definition of the origin_timestamp is a little confusing. It sounds very similar to the creation_timestamp (I appreciate IS-04 isn't clear on this). If it helps the ID and Timing work is aiming at a definition which includes just one core timestamp (which sounds like how you've defined the creation_timestamp). Any additional timestamps are likely to be part of the Flow payload.

11. The flow_id isn't currently included in the messages sent over the MQTT/WebSocket link, only the source_id. As I think I've mentioned elsewhere previously, this means it's impossible to work out the Flow ID associated with the data. The most straightforward solution would be to include both the source_id and the flow_id in the 'identity' part of the message.

12. Is there any way the 'type' definitions could be done as JSON schemas rather than using a freshly defined way of identifying maximums, minimums etc? I feel like I may have asked this previously, so it might be that this isn't possible for the same reason that IS-05 has to define its own constraints. If that's the case, then just ensuring as much consistency as possible with IS-05's constraints mechanism would be good so that they can be merged into JSON schemas to aid validation, rather than writing our own validators.

13. Where the WebSocket health messages are defined, would it be worth using the same default times as for IS-04? I note that a 5 second periodicity is already used for health messages, but the timeout is specified as when two consecutive health messages are missed. Could a specific time be associated with this, ie. 12 seconds is used for IS-04 to cover this two missed heartbeats plus 2 additional seconds to account for inaccurate clocks and transport latency.

14. It might be worth copying a few of the boilerplate API docs from the IS-04 repository, not least the CORS requirements and trailing slash policy in order to keep things consistent. In time we'll hopefully put this in a separate spec, but for now it might be sensible to be explicitly clear that this applies here too.

[cristian-recoseanu]

Hi Andrew,

I quick reply to number 10. One example is in the health messages for websocket.

The creation_timestamp is sent by the server at the time of generating the response whereas the origin_timestamp is the original timestamp sent by the client in the health command.

You could use this information to measure a rough round-trip time.

[mjeras]

Hi Andrew,
Regarding item 12 - the idea was to have something that would be simple to use and to allow for the recipient to easily figure out what the allowed values would be. If that would have been done through a JSON schema, it could potentially end up with very complex structures a vendor on the sending side could use in such a schema that would be easily used for validation, but not so much for getting the list of possible values on the receiving end. (e.g. there would be a number of ways of defining possible enum values in a JSON schema, especially using RegEx). The other thing I would like us to achieve in the future is easily building schemas for complex types what would just require the introduction of an "object and an "array" which would then allow for properties to be of one the basic types or another array or object (that was left out of the v1.0 spec to keep the scope limited). Unfortunately, that would not be compatible with the IS-05 way of defining constraints.
While I think that writing a validator that would validate such simple schemas (it is just three basic types with very limited constraints) is trivial, I will have a look if we might be able to define something that would be a subset of the JSON schema and make it immediately usable for validation (as it would be a JSON schema itself) or at least come with a straight forward translation of that into a valid JSON schema that could be then used for validation.
It might be also useful to discuss this over a call, maybe early next week by which time we should ideally be able to finalize all the details.

[garethsb]

I am very late to the party in reviewing Event & Tally, sorry - not enough bandwidth!

Andrew's item 12 jumped out at me as well. Using JSON Schema for type definitions, or a similar kind of cut-down JSON Schema that we adopted for IS-05 constraints, looks to me like it ought to work well here. The first thing that jumped out at me was seeing “min_length” rather than “minLength” for strings, and then the different way of defining enums, and rationals.

[garethsb]

broker_topic vs. rest_api_url

I'm not an MQTT expert; would it make sense to add "events" before "source" in the broker_topic, to clearly indicate this spec, and possibly the version too?

E.g.

{
  "broker_topic": "x-nmos/events/v1.0/sources/{sourceId}",
  "rest_api_url": "http://hostname/x-nmos/events/v1.0/sources/{sourceId}/"
}

I also wonder whether using "href", or "events_href", rather than "rest_api_url", would be more consistent with the naming of similar URL properties in IS-04?

[mjeras]

Regarding item 11: I have now added flow_id to the messages sent over one of the transports. Please note that no flow_id will be returned in the JSON when returning values through the REST API as there is not necessarily a 1:1 relationship between a source and a flow (the API is defined on the source as that is where the state is held).

[cristian-recoseanu]

Hi Gareth,

These parameter names are deprecated and as part of discussions with Andrew (he is looking at additions in IS-04 and IS-05) we have decided to go with some other names.

The more up to date versions are in my pull request here:

pull request here

[mjeras]

The first thing that jumped out at me was seeing “min_length” rather than “minLength” for strings

The spelling of that field was done with underscores so it would be compatible with the rest of NMOS where multi-word property names are spelled that way. The purpose of the type definition is to communicate the metadata related to the state in a simple and easy readable way and not to provide validation.

[garethsb]

@cristian-recoseanu > The more up to date versions are in my pull request

OK, thanks! I knew I had a lot to catch up with. :-)
The transport parameter names have got even longer; what's the reason for the "ext_is_07_" prefix?

@mjeras > The purpose of the type definition is to communicate the metadata related to the state in a simple and easy readable way and not to provide validation.

Neither are the IS-05 constraints about validation so much as setting expectation. It seems a good fit to me, but I may have missed some subtlety.

[cristian-recoseanu]

@garethsb-sony
ext_ is a prefix for externally defined parameters.
This is a conclusion reached after discussions with Andrew and the team behind IS-05 to allow for externally defined transport parameters that can be carried in the same patch request.
ext_ parameters will have their schemas defined in IS-07 as opposed to IS-05.
IS-05 maintains generic/general purpose use of parameters.
The schemas and most of everything else about these will follow the IS-05 way of doing things closely but they will sit within this repository as opposed to IS-05.