Add telemetry packets to JSON dictionary spec

[jwest115]

Related Issue(s)	#3147
Has Unit Tests (y/n)	n
Documentation Included (y/n)	y

---

Change Description

Updated the JSON dictionary specification to include telemetry packets and telemetry packet sets. Also cleaned up examples so they are more consistent/accurate.

[LeStarch]

Just the one question. Otherwise the packet stuff looks good. The other items look consistent with recent updates.

[LeStarch]

I would expect the packet name to be qualified, shouldn't I? i.e. M.T.P1.

[thomas-bc]

Agreed that it would likely make things easier on the GDS to have a qualified name

[bocchino]

We can make the name qualified if you want, but to me that seems unusual. The packet name is part of the packet set definition, similarly to the way a struct member name is part of a struct definition. In the past the convention we've used is that definitions use simple names and uses use qualified names. The issue with uses is that without the explicit qualification you have to do some name lookup. With definitions there is no such issue -- the name is qualified by the definition in which it appears.

[bocchino]

I think the name of the packet set in which the packet appears should be explicitly qualified by the (fully qualified) name of the topology in which it appears in the FPP source. Is that what we are currently doing?

[bocchino]

In the past the convention we've used is that definitions use simple names and uses use qualified names.

Unless the definition is stripped out of the qualifying context. So for example (I think) we use qualified names for the names of struct types but simple names for the members. We have to use qualified names for the types because the enclosing module context is stripped away when passing from the FPP source to the dictionary.

Here I think we need the module (and topology) context for the packet set. We could also add that to every packet in the set, but to me it seems overly verbose and maybe inconsistent.

[bocchino]

However, I think there's also an argument that we don't need the qualifier for the packet set (so the example is correct as is) because everything in the dictionary pertains to that topology. So the topology qualifier is redundant (indeed, everything in the dictionary could be qualified that way).

[bocchino]

When we generate C++ code, the packet sets will become classes, and their names will be appropriately qualified, because in C++ we don't assume that the code exists in an outer topology context.

[jwest115]

In my current implementation, telemetry packet set names are not fully qualified. This is also the case for the
set members/telemetry packet names. @LeStarch @thomas-bc is there a specific use case you have in mind for the GDS that would benefit from making these names fully qualified? After talking to Rob, we were thinking of adding a topology name field to the dictionary metadata (for ex: topologyName: M.T) which would make that information available in the dictionary.

[bocchino]

we were thinking of adding a topology name field to the dictionary metadata

The latest thinking is that the deployment name field should hold the fully-qualified topology name.

[bocchino]

When we generate C++ code, the packet sets will become classes

Note: In the current C++ implementation, the packet sets are stored as instances of a generic class called Svc::TlmPacketizerPacketList. The names are used for generating local variables that are hidden inside the implementation. There is no name qualification, and the names are not globally visible at all. This is true for both packets and packet sets.

[thomas-bc]

Agreed that it would likely make things easier on the GDS to have a qualified name

[bocchino]

Looks good! My only comment is that in the examples the instance names are written in CapitalCase. For stylistic consistency we may want to use camelCase. For example, instead of instance C1: C write instance c1: C. When I saw C1 used as an instance qualifier, my first thought was that C1 named a component rather than an instance.

[bocchino]

This looks good, but maybe we should keep the form "String representing ..." for consistency with the other fields? Or revise the other fields to match.