PoC: Interface for Firebolt Privacy Module #332
Conversation
MFransen69
requested review from
pwielders and
MFransen69
and removed request for
pwielders
interfaces/IFireboltPrivacy.h
Outdated
| ~IFireboltPrivacy() override = default; | ||
|
|
||
| // Pushing notifications to interested sinks | ||
| virtual uint32_t Register(IFireboltPrivacy::INotification* sink) = 0; |
There was a problem hiding this comment.
Please use Core::hresult as return type, if you want to know why give me a call
(and of course do that for all methods below)
interfaces/IFireboltPrivacy.h
Outdated
|
|
||
| // @property | ||
| // @brief Provides Current resume watch status | ||
| // @alt:AllowResumePoints |
There was a problem hiding this comment.
Why @alt, is this an existing interface?
There was a problem hiding this comment.
The method names in Firebolt spec are camelcased. In our thunder, all methods are made small. In the Thunder version that RDK has, @text is having an issue and is not changing the method name. Hence I used alt to achieve the same.
interfaces/IFireboltPrivacy.h
Outdated
| // @property | ||
| // @brief Provides Current resume watch status | ||
| // @alt:AllowResumePoints | ||
| virtual uint32_t AllowResumePoints(bool& allow /* @out */) const = 0; |
There was a problem hiding this comment.
As it mentions pointS what is the use case for this interface? Can there be multiple points?
If it it only for testing why not reuse another existing interface? Or is there really an existing use case?
There was a problem hiding this comment.
In Firebolt, the property name is called AllowResumePoints along with 's' hence the name.
interfaces/IFireboltPrivacy.h
Outdated
|
|
||
| // @brief Set resume watch status | ||
| // @alt:SetAllowResumePoints | ||
| virtual uint32_t SetAllowResumePoints(const bool& value ) = 0; |
There was a problem hiding this comment.
assuming the above and this method are a setter and getter (knowing the use case is helpful :) ) why is one a property and the other a method? (at least they should have the correct name and signature)
There was a problem hiding this comment.
I read in the Thunder documentation that the properties should not do any other work other than just changing the property value. But in Firebolt, the expectation is that it has to update the cloud components and have to send that value change event. Hence made the setter as method and getter doesn't need any extra operation so made that as a property.
interfaces/IFireboltPrivacy.h
Outdated
| enum { ID = ID_FIREBOLT_PRIVACY }; | ||
| enum StorageLocation : uint8_t { | ||
| Disk /* @text: Disk */, | ||
| InMemory /* @text: InMemory */, |
There was a problem hiding this comment.
No need to add the @text here, this is dthe default (PascalCase naming in JSON)
| virtual Core::hresult AllowResumePoints(bool& allow /* @out */) = 0; | ||
| // @brief sets the current resume watch status | ||
| // @text:SetAllowResumePoints | ||
| virtual Core::hresult SetAllowResumePoints(const bool& value ) = 0; |
There was a problem hiding this comment.
Oke to create both methods (i.s.o. properties) but please keep it consistent. Or both are prefixed with Get and Set or both are not prefixed (so no Get/Set in front of it).
interfaces/IFireboltPrivacy.h
Outdated
|
|
||
| // @property | ||
| // @brief Get the storage location | ||
| virtual Core::hresult GetStorageLocation(StorageLocation& value /* @out */) const = 0; |
There was a problem hiding this comment.
If you remove Get/Set from the other methods remove it here as well for consistency.
Is there a "real" use-case for knowing where the configurations where stored ? Do we really need this method ?
Please explain the usecase .
There was a problem hiding this comment.
Yes, there is no real usecase based on this. Since we are moving the configuration from the device-manifest file (ripple configuration) to plugin configuration, I thought if ripple wants to know the stored location for logging this API would be useful.
interfaces/IFireboltPrivacy.h
Outdated
|
|
||
| // @brief Provides Current resume watch status | ||
| // @text:AllowResumePoints | ||
| virtual Core::hresult AllowResumePoints(bool& allow /* @out */) = 0; |
There was a problem hiding this comment.
I guess this is the getter, why is it not const ?
Correcting member datatypes in Discoverypolicy
Removing unused struct
No description provided.