Json meta to IDL changes #394

HaseenaSainul · 2024-11-05T07:04:39Z

No description provided.

pwielders · 2024-11-05T07:30:11Z

definitions/CMakeLists.txt

@@ -35,6 +35,16 @@ set(WORKING_VARIABLE ${JSONRPC_PATTERNS})
 list(TRANSFORM WORKING_VARIABLE PREPEND "${CMAKE_SOURCE_DIR}/jsonrpc/")
 file(GLOB JSON_FILE ${WORKING_VARIABLE})

+if(NOT ENABLE_LEGACY_INTERFACE_SUPPORT)


I hope we can just remove these json files from the repository... so no need to remove them only from the Cmake. If you remove them from the directory they will aslo not end up in the list..

Ok, but actually I have added this flag to retain the json files to support older interfaces

interfaces/IDeviceInfo.h

pwielders · 2024-11-06T14:01:49Z

interfaces/IDeviceInfo.h

+
+        // @property
+        // @brief Retrieves Device Info
+        virtual Core::hresult DeviceData(Device& device /* @out */) const = 0;


Maybe for consistency, if possible, call these methods like the interface bu than without the I, so:
virtual Core::hresult DeviceMetadata(Device& device /* @out */) const = 0;

pwielders · 2024-11-06T14:02:11Z

interfaces/IDeviceInfo.h

+
+        // @property
+        // @brief Retrieves Firware Information
+        virtual Core::hresult FirmwareInfo(Firmware& firmware /* @out */) const = 0;


Maybe for consistency, if possible, call these methods like the interface bu than without the I, so:
virtual Core::hresult ImageMetadata(Firmware& device /* @out */) const = 0;

pwielders · 2024-11-06T14:02:37Z

interfaces/IDeviceInfo.h

+
+        // @property
+        // @brief Retrieves SystemInfo
+        virtual Core::hresult SystemInfo(System& system /* @out */) const = 0;


Maybe for consistency, if possible, call these methods like the interface bu than without the I, so:
virtual Core::hresult SystemMetadata(System& device /* @out */) const = 0;

pwielders · 2024-11-06T14:03:03Z

interfaces/IDeviceInfo.h

+
+        // @property
+        // @brief Retrieves SocketInfo
+        virtual Core::hresult SocketInfo(Socket& socket /* @out */) const = 0;


Maybe for consistency, if possible, call these methods like the interface bu than without the I, so:
virtual Core::hresult SocketMetadata(Socket& device /* @out */) const = 0;

pwielders · 2024-11-06T14:03:29Z

interfaces/IDeviceInfo.h

+        // @property
+        // @brief Retrieves AddressInfo
+        // @param addresses: An array of Interface address
+        virtual Core::hresult AddressInfo(IAddressIterator*& ip /* @out */) const = 0;


Maybe for consistency, if possible, call these methods like the interface bu than without the I, so:
virtual Core::hresult AddressMetadata(IAddressIterator*& ip /* @out */ const = 0;

pwielders · 2024-11-06T14:11:47Z

interfaces/IDeviceInfo.h

@@ -25,136 +25,315 @@

 namespace Thunder {
 namespace Exchange {
-
+    /* @json 1.0.0 */


I think you can drop the annotation /* @json 1.0.0 */ on this interface and even mark it deprecated..

as per the discussion it is still required to make sure jsonrpc interfaces with backward compatible

pwielders · 2024-11-06T14:17:35Z

interfaces/IMonitor.h

+
+        using IStatisticsIterator = RPC::IIteratorType<Statistics, ID_MONITOR_STATISTICS_ITERATOR>;
+        // @brief RestartLimits: Sets new restart limits for a plugin
+        virtual uint32_t RestartLimits(const RestartLimitsInfo& params) = 0;


Why not:

virtual Core::hresult RestartLimits(const string callsign /* @index */, const RestartLimitsInfo& params) = 0; virtual Core::hresult RestartLimits(const string callsign /* @index */, RestartLimitsInfo& params) const = 0;

so we get a property in JSON to set and get these ? and we do not need the additional struct RestartLimitsInfo and as we know have a proeprty to get the Restart limits, you can also drop the struct Statistics

pwielders · 2024-11-06T14:23:04Z

interfaces/IMonitor.h

+        virtual uint32_t RestartLimits(const RestartLimitsInfo& params) = 0;
+        // @brief ResetStats: Resets memory and process statistics for a single plugin watched by the Monitor
+        virtual uint32_t ResetStats(const string& callsign, Statistics& statistics /* @out */) = 0;
+        // @property


It does not reset the restartInformation, only the measurements, so suggest:

virtual Core::hresult Reset(const string& callsign, Measurements& currentMeasurements /* @out */) = 0;

It will do the reset, so I changed the interface to support only reset : virtual uint32_t Reset(const string& callsign) = 0;, the status can be retrieve with other interface Status()

pwielders · 2024-11-06T14:28:28Z

interfaces/IMonitor.h

+            RestartInfo restart /* @brief Restart limits for failures applying to the service */;
+        };
+
+        struct EXTERNAL INotification : virtual public Core::IUnknown {


I guess we need a limited set of actions the Monitor can do, so lets create an enum for all possible actions, the Monitor can do within this interface. Than lets use the enum in the Action method below..
Do not know what the reason typically is, but I guess that might also be a very limited set, so why not turn that into an enum as well ?

We are "defining" a COMRPC interface, so I guess this interface, an INotification, needs a Register and Unregister as methods below :-)

pwielders · 2024-11-06T14:29:58Z

interfaces/IMonitor.h

+            string observable /* @brief A callsign of the watched service */;
+            RestartInfo restart /* @brief Restart limits for failures applying to the service */;
+        };
+        struct ActionParams {


I think this whole struct can be dropped. It is used no where.

pwielders · 2024-11-06T14:30:33Z

interfaces/IMonitor.h

+            string action /* @brief The action executed by the Monitor on a service. One of: "Activate", "Deactivate", "StoppedRestarting" */;
+            string reason /* @brief A message describing the reason the action was taken */;
+        };
+        struct RestartLimitsInfo {


If we make the methods below more strict, this structure can be dropped as well.

pwielders · 2024-11-06T14:31:46Z

interfaces/IMonitor.h

+            uint64_t average /* @brief Average of all measurements */;
+            uint64_t last /* @brief Last measured value */;
+        };
+        struct Measurements {


Suggest to call this statistics as Measurement and Measurements is quite close to each other and I think we bdio not need the Statistics struct below, with more strong typed interface suggestions at the end of this file.

done, I have kept Statistics as the name for Measurements

pwielders · 2024-11-06T14:50:50Z

interfaces/IMonitor.h

+        // @property
+        // @brief Status: The memory and process statistics either for a single plugin or
+        // all plugins watched by the Monitor
+        virtual uint32_t Status(const string& callsign /* @index */, IStatisticsIterator*& statistics /* @out */) const = 0;


I am a bit in doubt. If we want to keep it like this, it should be like:

virtual Core::hresult Status(const Core::OptionalType<string>& callsign /* @index */, IStatisticsIterator*& statistics /* @out */) const = 0;

However if you look at how this is being used:

I think the folowing interface is more suitable and does not require the struct Statistics you have today and it is more inline with a potential to reduce the buld of data moved over the line.

Suggest the following interface:

virtual Core::hresult Observables(IStringIterator*& observables /* @out */) const = 0; virtual Core::hresult Statistics (const string& callsign /* @index */, Measurements*& statistics /* @out */) const = 0; // if you rename Measurements to Statistics this offcourse becomes: virtual Core::hresult Statistics (const string& callsign /* @index */, Statistics*& statistics /* @out */) const

pwielders · 2024-11-06T14:55:58Z

interfaces/IOCDM.h

+    // @alt Drms
+    // @property
+    // @brief Systems - Retrieves all key systems available in the system (e.g. Nagra, PlayReady, WideVine etc)
+    virtual Core::hresult Systems(IDrmIterator*& drms /* @out */) const = 0;


I think this returns a list of "keySystems", so I think this should be:

virtual Core::hresult Systems(IStringIterator*& keySystems /* @out */) const = 0;

The Drm struct is not needed at all...

If it is for Backawards compatibility, just continue to use the Register("Drms") method, as it is today with its specific code.. We can than deprecate this method and in due time drop it.

pwielders · 2024-11-06T15:11:17Z

interfaces/ISecurityAgent.h

+            string hash /* @brief Random hash */;
+        };
+
+        // @brief Creates Token


From a security point of view, I think we do not want the Create Token to be on the JSONRPC interface. Than anyone could create any token. Security wise, I do not think this is a good idea.. Dd you find a JSON file that eposeds this interface method ?

It also looks a bit like a "dedicated" Comcast specific functionality. The creation of the token should take a BLOB. The Create TOken Process is an abstract ed metod to just sign a BLOB. Whats in the BLOB is not interesting and shoudl not be exposed on the interface by this struct..

pwielders · 2024-11-06T15:13:54Z

interfaces/ISecurityAgent.h

+        // @brief Creates Token
+        virtual uint32_t CreateToken(const TokenInput& input, string& token /* @out */) = 0;
+        // @brief Validate Token
+        virtual uint32_t Validate(const string& token, bool& valid /* @out */) = 0;


Methods as of R4.1 should return Core::hresult :-)

pwielders · 2024-11-06T15:17:44Z

interfaces/IStateControl.h

+
+namespace Exchange {
+
+    namespace JSONRPC {


Is there a reason why we not add the annotation /* @json */ here : https://github.com/rdkcentral/Thunder/blob/master/Source/plugins/IStateControl.h#L31

pwielders · 2024-11-06T15:19:59Z

interfaces/IBrowser.h

            virtual void BridgeQuery(const string& message) = 0;
+            // @brief Signals a state change of the Browser


I guess this is from the IStateControl interface. Is it not possible to get it from that interface, see: https://github.com/rdkcentral/Thunder/blob/master/Source/plugins/IStateControl.h#L49

pwielders · 2024-11-06T15:21:37Z

interfaces/IBrowser.h

@@ -149,6 +161,17 @@ namespace Exchange {

        // @brief Initiate garbage collection
        virtual uint32_t CollectGarbage() = 0;
+        // @brief Delete directory
+	// @param path: Path to be deleted
+        virtual uint32_t Delete(const string& path) = 0;


This on JSONRPC is not a very good option. Did you find it somewhere in *.json file ?

sebaszm · 2024-11-08T10:53:41Z

definitions/Definitions.cpp

@@ -186,6 +186,7 @@ ENUM_CONVERSION_BEGIN(Exchange::IBrightness::Brightness)
    { Exchange::IBrightness::SdrToHdrGraphicsBrightness_Max, _TXT("max") },
 ENUM_CONVERSION_END(Exchange::IBrightness::Brightness)

+#if ENABLE_LEGACY_INTERFACE_SUPPORT


The enumeration tables for enums that a part of interfaces that are tagged (at)json are (should be!) redundant here.

sebaszm · 2024-11-08T10:57:38Z

interfaces/IBrowser.h

@@ -114,9 +126,9 @@ namespace Exchange {
        // @param fps: Current FPS
        virtual uint32_t FPS(uint8_t& fps /* @out */) const = 0;

-        /* @json:omit */
+        /* @json:omit @alt:deprecated */


alt is for json-rpc only but here it's omitted, it doesn't make sense together. I think what you mean is to deprecate the C++ method, i.e. mark it DEPRECATED
DEPRECATED virtual uint32_t HeaderList(string& headerlist /* @out */) const = 0;

sebaszm · 2024-11-08T11:00:38Z

interfaces/IBrowser.h

@@ -149,6 +161,14 @@ namespace Exchange {

        // @brief Initiate garbage collection
        virtual uint32_t CollectGarbage() = 0;
+        // @property
+        // @brief Languages: Browser prefered languages


Don't prepend brief with the method name, the generator already knows it.

sebaszm · 2024-11-08T11:02:28Z

interfaces/IBrowser.h

@@ -149,6 +161,14 @@ namespace Exchange {

        // @brief Initiate garbage collection
        virtual uint32_t CollectGarbage() = 0;
+        // @property
+        // @brief Languages: Browser prefered languages


Don't prepend brief with the method name, the generator already knows it.

sebaszm · 2024-11-08T11:02:42Z

interfaces/IMonitor.h

+        virtual Core::hresult Unregister(INotification* notification) = 0;
+
+        // @property
+        // @brief RestartLimits: Set/Get new restart limits for a plugin


Again this will look odd in the documentation, ie. no need to repeat the method name.

sebaszm · 2024-11-08T11:25:07Z

interfaces/IDeviceInfo.h

+        virtual ~IImageMetadata() override = default;
+
+        struct Firmware {
+            enum Yocto : uint8_t {


Let's reconsider if we want to have the build system as an enum...

sebaszm · 2024-11-08T11:26:40Z

interfaces/IDeviceInfo.h

+        struct AudioOutputCaps {
+            IDeviceAudioCapabilities::AudioOutput audioOutput /* @brief Audio Output support */;
+            IDeviceAudioCapabilities::AudioCapability audioCapabilities /* @bitmask @brief Retrieves AudioCapabilities */;
+            IDeviceAudioCapabilities::MS12Capability ms12Capabilities /* @bitmask @brief Retrieves MS12 Capabilities */;


I think ms12 support is not always available, so these could be OptionalType<>

interfaces/IDeviceInfo.h

sebaszm · 2024-11-08T11:30:33Z

interfaces/IDeviceInfo.h

+
+        struct Device {
+            string deviceType /* @brief Device type */;
+            string distributorId /* @brief Partner ID or distributor ID for device */;


Some of these fields seem optional... distributorid, friendlyname, sku... ?

sebaszm · 2024-11-08T11:32:04Z

interfaces/IDeviceInfo.h

+
+            string imageName /* @brief Name of firmware image */;
+            string sdk /* @brief SDK version string */;
+            string mediarite /* @brief Mediarite value */;


I suppose sdk and mediarite are optional

pwielders reviewed Nov 5, 2024

View reviewed changes

interfaces/IDeviceInfo.h Outdated Show resolved Hide resolved

pwielders reviewed Nov 5, 2024

View reviewed changes

interfaces/IDeviceInfo.h Outdated Show resolved Hide resolved

pwielders reviewed Nov 5, 2024

View reviewed changes

interfaces/IDeviceInfo.h Show resolved Hide resolved

HaseenaSainul force-pushed the development/METROL-1071 branch 2 times, most recently from 4d86f68 to 8c69e90 Compare November 6, 2024 10:00

pwielders reviewed Nov 6, 2024

View reviewed changes

sebaszm self-requested a review November 8, 2024 10:20

sebaszm requested changes Nov 8, 2024

View reviewed changes

HaseenaSainul added 3 commits November 8, 2024 07:03

Json meta to IDL changes

b078162

Json meta to IDL changes in progress

5131a81

Json meta to IDL changes in progress

04b94cd

HaseenaSainul added 2 commits November 8, 2024 07:03

Json meta to IDL changes in progress

7ecd449

Json meta to IDL changes in progress

44ef49a

HaseenaSainul force-pushed the development/METROL-1071 branch from 7a79de7 to 44ef49a Compare November 8, 2024 12:03

		virtual void BridgeQuery(const string& message) = 0;
		// @brief Signals a state change of the Browser

Json meta to IDL changes #394

Are you sure you want to change the base?

Json meta to IDL changes #394

Conversation

HaseenaSainul commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwielders Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwielders Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwielders Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwielders Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwielders Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwielders Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwielders Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

pwielders Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwielders Nov 6, 2024 •

edited

Loading

pwielders Nov 6, 2024 •

edited

Loading

pwielders Nov 6, 2024 •

edited

Loading

pwielders Nov 6, 2024 •

edited

Loading

pwielders Nov 6, 2024 •

edited

Loading

pwielders Nov 6, 2024 •

edited

Loading

pwielders Nov 6, 2024 •

edited

Loading

pwielders Nov 6, 2024 •

edited

Loading