Json meta to IDL changes #394
Conversation
definitions/CMakeLists.txt
Outdated
| @@ -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) | |||
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
Ok, but actually I have added this flag to retain the json files to support older interfaces
HaseenaSainul
force-pushed
the
development/METROL-1071
branch
2 times, most recently
from
4d86f68 to
8c69e90
Compare
interfaces/IDeviceInfo.h
Outdated
|
|
||
| // @property | ||
| // @brief Retrieves Device Info | ||
| virtual Core::hresult DeviceData(Device& device /* @out */) const = 0; |
There was a problem hiding this comment.
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;
interfaces/IDeviceInfo.h
Outdated
|
|
||
| // @property | ||
| // @brief Retrieves Firware Information | ||
| virtual Core::hresult FirmwareInfo(Firmware& firmware /* @out */) const = 0; |
There was a problem hiding this comment.
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;
interfaces/IDeviceInfo.h
Outdated
|
|
||
| // @property | ||
| // @brief Retrieves SystemInfo | ||
| virtual Core::hresult SystemInfo(System& system /* @out */) const = 0; |
There was a problem hiding this comment.
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;
interfaces/IDeviceInfo.h
Outdated
|
|
||
| // @property | ||
| // @brief Retrieves SocketInfo | ||
| virtual Core::hresult SocketInfo(Socket& socket /* @out */) const = 0; |
There was a problem hiding this comment.
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;
interfaces/IDeviceInfo.h
Outdated
| // @property | ||
| // @brief Retrieves AddressInfo | ||
| // @param addresses: An array of Interface address | ||
| virtual Core::hresult AddressInfo(IAddressIterator*& ip /* @out */) const = 0; |
There was a problem hiding this comment.
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;
interfaces/IDeviceInfo.h
Outdated
| @@ -25,136 +25,315 @@ | |||
|
|
|||
| namespace Thunder { | |||
| namespace Exchange { | |||
|
|
|||
| /* @json 1.0.0 */ | |||
There was a problem hiding this comment.
I think you can drop the annotation /* @json 1.0.0 */ on this interface and even mark it deprecated..
There was a problem hiding this comment.
as per the discussion it is still required to make sure jsonrpc interfaces with backward compatible
interfaces/IOCDM.h
Outdated
| // @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; |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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..
interfaces/ISecurityAgent.h
Outdated
| // @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; |
There was a problem hiding this comment.
Methods as of R4.1 should return Core::hresult :-)
interfaces/IStateControl.h
Outdated
|
|
||
| namespace Exchange { | ||
|
|
||
| namespace JSONRPC { |
There was a problem hiding this comment.
Is there a reason why we not add the annotation /* @json */ here : https://github.com/rdkcentral/Thunder/blob/master/Source/plugins/IStateControl.h#L31
interfaces/IBrowser.h
Outdated
| virtual void BridgeQuery(const string& message) = 0; | ||
| // @brief Signals a state change of the Browser |
There was a problem hiding this comment.
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
interfaces/IBrowser.h
Outdated
| @@ -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; | |||
There was a problem hiding this comment.
This on JSONRPC is not a very good option. Did you find it somewhere in *.json file ?
| @@ -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 | |||
There was a problem hiding this comment.
The enumeration tables for enums that a part of interfaces that are tagged (at)json are (should be!) redundant here.
interfaces/IBrowser.h
Outdated
| @@ -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 */ | |||
There was a problem hiding this comment.
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;
interfaces/IBrowser.h
Outdated
| @@ -149,6 +161,14 @@ namespace Exchange { | |||
|
|
|||
| // @brief Initiate garbage collection | |||
| virtual uint32_t CollectGarbage() = 0; | |||
| // @property | |||
| // @brief Languages: Browser prefered languages | |||
There was a problem hiding this comment.
Don't prepend brief with the method name, the generator already knows it.
interfaces/IBrowser.h
Outdated
| @@ -149,6 +161,14 @@ namespace Exchange { | |||
|
|
|||
| // @brief Initiate garbage collection | |||
| virtual uint32_t CollectGarbage() = 0; | |||
| // @property | |||
| // @brief Languages: Browser prefered languages | |||
There was a problem hiding this comment.
Don't prepend brief with the method name, the generator already knows it.
interfaces/IDeviceInfo.h
Outdated
| virtual ~IImageMetadata() override = default; | ||
|
|
||
| struct Firmware { | ||
| enum Yocto : uint8_t { |
There was a problem hiding this comment.
Let's reconsider if we want to have the build system as an enum...
interfaces/IDeviceInfo.h
Outdated
| struct AudioOutputCaps { | ||
| IDeviceAudioCapabilities::AudioOutput audioOutput /* @brief Audio Output support */; | ||
| IDeviceAudioCapabilities::AudioCapability audioCapabilities /* @bitmask @brief Retrieves AudioCapabilities */; | ||
| IDeviceAudioCapabilities::MS12Capability ms12Capabilities /* @bitmask @brief Retrieves MS12 Capabilities */; |
There was a problem hiding this comment.
I think ms12 support is not always available, so these could be OptionalType<>
interfaces/IDeviceInfo.h
Outdated
|
|
||
| struct Device { | ||
| string deviceType /* @brief Device type */; | ||
| string distributorId /* @brief Partner ID or distributor ID for device */; |
There was a problem hiding this comment.
Some of these fields seem optional... distributorid, friendlyname, sku... ?
interfaces/IDeviceInfo.h
Outdated
|
|
||
| string imageName /* @brief Name of firmware image */; | ||
| string sdk /* @brief SDK version string */; | ||
| string mediarite /* @brief Mediarite value */; |
There was a problem hiding this comment.
I suppose sdk and mediarite are optional
HaseenaSainul
force-pushed
the
development/METROL-1071
branch
from
7a79de7 to
44ef49a
Compare
HaseenaSainul
force-pushed
the
development/METROL-1071
branch
from
44ef49a to
c05db9f
Compare
HaseenaSainul
force-pushed
the
development/METROL-1071
branch
from
276172d to
78ffa9e
Compare
No description provided.