Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Json meta to IDL changes #394

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

HaseenaSainul
Copy link
Contributor

No description provided.

@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

interfaces/IDeviceInfo.h Outdated Show resolved Hide resolved
interfaces/IDeviceInfo.h Outdated Show resolved Hide resolved
interfaces/IDeviceInfo.h Outdated Show resolved Hide resolved
@HaseenaSainul HaseenaSainul force-pushed the development/METROL-1071 branch 2 times, most recently from 4d86f68 to 8c69e90 Compare November 6, 2024 10:00

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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

@pwielders pwielders Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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

namespace Thunder {
namespace Exchange {

/* @json 1.0.0 */
Copy link
Collaborator

@pwielders pwielders Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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;
Copy link
Collaborator

@pwielders pwielders Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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
Copy link
Collaborator

@pwielders pwielders Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

RestartInfo restart /* @brief Restart limits for failures applying to the service */;
};

struct EXTERNAL INotification : virtual public Core::IUnknown {
Copy link
Collaborator

@pwielders pwielders Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, I have kept Statistics as the name for Measurements

// @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;
Copy link
Collaborator

@pwielders pwielders Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
image

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   

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

// @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;
Copy link
Collaborator

@pwielders pwielders Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

string hash /* @brief Random hash */;
};

// @brief Creates Token
Copy link
Collaborator

@pwielders pwielders Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

// @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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


namespace Exchange {

namespace JSONRPC {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sebaszm sebaszm self-requested a review November 8, 2024 10:20
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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 */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

virtual Core::hresult Unregister(INotification* notification) = 0;

// @property
// @brief RestartLimits: Set/Get new restart limits for a plugin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

virtual ~IImageMetadata() override = default;

struct Firmware {
enum Yocto : uint8_t {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

interfaces/IDeviceInfo.h Outdated Show resolved Hide resolved

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose sdk and mediarite are optional

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@HaseenaSainul HaseenaSainul force-pushed the development/METROL-1071 branch from 7a79de7 to 44ef49a Compare November 8, 2024 12:03
@HaseenaSainul HaseenaSainul force-pushed the development/METROL-1071 branch from 44ef49a to c05db9f Compare November 20, 2024 08:51
@HaseenaSainul HaseenaSainul force-pushed the development/METROL-1071 branch from 276172d to 78ffa9e Compare November 28, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants