[docs] : add 'guidelines' in 'plugins/interfaces' #1721
Conversation
msieben
force-pushed
the
development/documentation
branch
from
aa8f738
to
be21078
Compare
|
|
I've received some initial feedback which I'll add at the appropriate lines |
docs/plugin/interfaces/guidelines.md
Outdated
| Comply to the Single Responsibility Principle (SPR) {#ComplyToSingleResponsibilityPrinciple} | ||
| --------------------------------------------------- | ||
|
|
||
| The Single Responsibility Principle (SRP) is a fundamental principle in software development that states that an interface should have only one reason to change, which should also adhere to the idea of having a single, well-defined responsibility. Here are some reasons why interfaces should comply with the Single Responsibility Principle: |
There was a problem hiding this comment.
Could this be reworded to make it easier to understand.
|
|
||
| When designing an interface make sure not only to focus on the static behaviour, meaning what methods and notifications are necessary to make sure the client can use it for its purpose, but also on the dynamic behaviour, meaning how should the interface behave when the methods are called at certain times in a certain order and when certain notifications are sent in between (also see the "Less is more" paragraph above on advise on how to reduce the complexity of the dynamic behaviour). This of course also taking into account that the embedded domain we are operating in with Thunder allows for calls being done from multiple applications and/or threads so any call can happen in parallel with any other or any notification. | ||
|
|
||
| Think about data that needs to be changed and/or read atomically. For example it may look logical in some case to have setters/getters for individual items but if the overall consistency of some items combined needs to be guarded it may make more sense to combine these items in one getter and setter, this prevents one application writing ValueA and then ValueB, while another one reads both A and B in between the two writes and is reading an inconsistent set. |
There was a problem hiding this comment.
Would it help to add diagrams?
| Also performance may be an important aspect to take into consideration when thinking about the (dynamic) behaviour of the interface. Suppose an interface is created for storing key value pairs and one expects a client to change and/or read a lot of values after one each other and the writing and reading is expected to be quite resource intensive (for example write take some time) it could be a consideration to make it possible to write/read multiple pairs in one call to prevent the client of creating quite a number of parallel calls that queue up in the framework and or plugin if the client for example would do this from multiple threads or would use CURL or a similar solution to do these calls in parallel. Or depending on the situation and expected usage pattern perhaps make the interface asynchronous so the calls itself will return quickly while the implementation can now handle multiple writes more efficiently by combining them (of course still with some maximum to cap the number of maximum parallel requests to prevent a too high of a peak in resource usage). | ||
| In any case make sure to next to the static behaviour of the interface also document the expected dynamic behaviour so that both the implementer of the interface as well as the user know what to expect. | ||
|
|
||
| Also for notifications one should think very carefully how they behave dynamically and what data the notifications do carry. For example a naive idea might be to always sent a notification on a data change event including the new value of the data. Although this may work perfectly if the data does not change much it will have a big influence on the expected behaviour if the data changes often and the consistency of the data read must be guaranteed. |
There was a problem hiding this comment.
Would it help to add diagrams here as well?
docs/plugin/interfaces/guidelines.md
Outdated
|
|
||
| To exploit these advantages to the fullest, Thunder uses Interfaces definitions between plugins and internally. The interface definitions created for Thunder should adhere to certain rules/recommendations and guidelines. This document describes these Rules/Recommendations and guidelines for designing a good interface and the syntax to be used to describe the interface. | ||
|
|
||
| Rule, Recommendation or guideline {#RuleRecommendationGuideline} |
| * Guidelines are more flexible than rules, as they offer suggestions on the most effective way to accomplish a task or goal. | ||
| * They serve as a framework for good practice and can be adapted or customized based on specific circumstances or requirements. | ||
|
|
||
| In summary, rules are strict, mandatory, and non-negotiable directives, recommendations are suggestions or advice that are not obligatory, and guidelines offer flexible principles for achieving a specific objective. |
There was a problem hiding this comment.
The worded distinction between recommendation and guideline isn't very clear.
Especially because it is mentioned that recommendations can be a guidelines.
The concrete examples later provide better insight, but wording could be improved here.
In the sections below, the order is different: Rule, Guideline, Recommendation.
Would be great to be consistent in both places.
docs/plugin/interfaces/guidelines.md
Outdated
|
|
||
| * **Encapsulation**: By hiding implementation details, interfaces ensure that the internal workings of a system or component are not exposed to the outside world. This promotes encapsulation, which is a core principle of object-oriented design. | ||
| * **Flexibility and Maintainability**: When interfaces do not expose implementation details, the underlying code can be changed, optimized, or refactored without affecting the code that depends on the interface. This makes the system more adaptable and easier to maintain. | ||
| * **Abstraction**: Interfaces should provide a high-level abstraction that defines what operations can be performed without detailing how they are performed. This allows developers to focus on the "what" rather than the "how," making the system easier to understand and use. |
There was a problem hiding this comment.
'developers' in this sense are interface users, not interface implementers.
Probably worth making that clear to avoid confusion.
| * **Interchangeability**: When interfaces do not leak implementation details, it becomes easier to interchange different implementations of the same interface. This is particularly useful in scenarios where multiple implementations might be needed, such as testing with mock objects or switching between different service providers. | ||
| * **Simpler API**: A clean interface that hides complexity makes the API simpler and more intuitive to use. Users of the interface do not need to understand the complexities behind the scenes, leading to a better developer experience. | ||
|
|
||
| To achieve these benefits, interfaces should be carefully designed to define clear and minimal contracts for interaction, focusing on what needs to be done rather than how it is done. |
There was a problem hiding this comment.
I'm wondering if this list of rules could be turned into a set of Interface Design Goals as a checklist for interface developers to easily run through?
The guidelines have all the information needed for developers, but it is quite passively wordy.
Sometimes a checklist which asks questions of the reader can be helpful to engage them.
e.g. Simpler API - Is there a way to cut the number of methods or parameters in the interface to reduce its API surface, making it easier to implement and test?
|
|
||
| In summary, rules are strict, mandatory, and non-negotiable directives, recommendations are suggestions or advice that are not obligatory, and guidelines offer flexible principles for achieving a specific objective. | ||
|
|
||
| Rules {#Rules} |
There was a problem hiding this comment.
Do we also want rules on documentation style/quality, method/variable naming convention and mandatory test authoring?
docs/plugin/interfaces/guidelines.md
Outdated
| }; | ||
| ``` | ||
|
|
||
| A ***struct*** in the ***C programming language*** (and many derivatives) is a ***composite data type*** (or ***record***) declaration that defines a physically grouped list of variables under one name in a block of memory, allowing the different variables to be accessed via a single ***pointer*** or by the struct declared name which returns the same address. The struct data type can contain other data types so is used for mixed-data-type records such as a hard-drive directory entry (file length, name, extension, physical address, etc.), or other mixed-type records (name, address, telephone, balance, etc.). |
There was a problem hiding this comment.
Good info on struct in C, but no comparison to its use here in C++, which acts like a class with default public access.
docs/plugin/interfaces/guidelines.md
Outdated
| As an interface is in principle immutable it is better to spend more time on this before releasing the interface than later on trying to improve the interface or writing a lot of documentation explaining on how to use it in certain situations. | ||
|
|
||
| When designing the interface it is good to use the "less it more" principle when adding methods to it to make sure it can do what should be possible with it. | ||
| Less methods will make it easier to understand the interface, less documentation will be needed, it is easier to keep it consistent and there is less need to think or document what will happen if you use the methods in an unexpected order. |
| Take both Static and Dynamic behaviour into account {#StaticAndDynamic} | ||
| ------------------------ | ||
|
|
||
| When designing an interface make sure not only to focus on the static behaviour, meaning what methods and notifications are necessary to make sure the client can use it for its purpose, but also on the dynamic behaviour, meaning how should the interface behave when the methods are called at certain times in a certain order and when certain notifications are sent in between (also see the "Less is more" paragraph above on advise on how to reduce the complexity of the dynamic behaviour). This of course also taking into account that the embedded domain we are operating in with Thunder allows for calls being done from multiple applications and/or threads so any call can happen in parallel with any other or any notification. |
There was a problem hiding this comment.
Use of the @pre for prerequisite conditions before calling a method could be useful here, but couldn't see it in the tag list.
It's part of doxygen though.
At least, any dependent behavior should be called out in the API documentation.
|
|
||
| Sometimes when designing an interface it can be expected that the implementation of a certain action can be quite expensive and take some time. | ||
|
|
||
| In such a case (we usually use an expected duration of a few hundred milliseconds as the limit) or when the duration of an action is non deterministic and worse case would take longer than a few hundred milliseconds, it is advised to make the interface asynchronous. The interface will then only pass the data needed to start the action to the component and add a callback to the interface to make it possible for the implementation to return the result asynchronously. The component will handle the action in the background (do not use threads for this but use the Thunder workerpool) and call the provided callback when the action is completed to signal this to the initiator of the action. Not only will this prevent the call to fail because of JSON-RPC or COM-RPC timeouts but it will also make sure the framework is not blocked internally and also the client can continue without being blocked waiting for the result (or needed to reserve a thread for this). |
There was a problem hiding this comment.
I would word the limit on method execution times more strongly.
e.g. Any call could take more than 300ms must use an asynchronous pattern.
Probably worth making developers aware of the JSON-RPC and COM-RPC timeout values.
|
|
||
| Also note, as was pointed out in the "State complete interfaces" paragraph of this page, that there should also be a means to stop the initiated action before it has completed. This is for example needed when the component that started the action needs to shut down and with this it can signal that the action can be stopped as it is no longer needed and the provided callback is no longer valid and should not longer be called. | ||
|
|
||
|
|
There was a problem hiding this comment.
Do we need guidance on choice of JSON over COM when exposing an interface?
There was a problem hiding this comment.
Hi @lucienkennedylamb see if I understand your remark correctly: you mean when as a 'customer of the interface' you should use one over the other? Or when as an interface provider decide if to provide both or only COM-RPC?
There was a problem hiding this comment.
I was alluding to the interface developer - choosing to provide both or only COM-RPC, so I'm wondering if we should provide guidance on how to make that decision.
But actually you're right about the 'customer' or client of an interface too - If they have both options available to them should they always go with COM-RPC over JSON-RPC which provides stronger typing?
There was a problem hiding this comment.
Thanks for clearing this up, I'll add some text to describe at least the options and when you should consider what.
On your question for the customer of the interface our advise would be to use COM-RPC for the native world, it is strong typed and binary (so will reduce resource usage). JSON-RPC is targeted at the webworld where that is much easier to use.
| |0x80000000|0xA0000000|Custom Plugin interfaces|```<Custom>```| | ||
| |0xA0000000|0xFFFFFFFF|QA Interfaces|```ThunderInterfaces/interfaces/ids.h```| | ||
|
|
||
| Tags/annotations available for optimizing the generated code {#TagsAnnotationsAvailableForOptimizingTheGeneratedCode} |
There was a problem hiding this comment.
Hi @anand-ky we cannot make it the default as that would break backwards compatibility for a lot of existing interfaces.
We think the mandatory rules for these kind of items (e.g. what is mandatory to document, casing used, naming etc.) should be in a separate document that list them for RDK-E, let's discuss this in the next interfaces meeting.
docs/plugin/interfaces/guidelines.md
Outdated
|
|
||
| Although the const bool in the second method has no added value, it is a recommendation to use it, just to indicate clearly that the intend is that the implementation should not and can not change the parameter. | ||
|
|
||
| !!! note "TODO" |
There was a problem hiding this comment.
I see there is a Todo for properties. Just adding that we should provide guidelines on when to use properties vs specific getter/setter. I see some info under @Property tag. Maybe include that in the guidelines here?
| Tags/annotations available for optimizing the generated code {#TagsAnnotationsAvailableForOptimizingTheGeneratedCode} | ||
| ============================================================ | ||
|
|
||
| Tags/annotations that can be used to optimize the generated code, influence the generated JSON-RPC interface from the interface (if desired at all), or document the interface can be found here: |
There was a problem hiding this comment.
Could we add a section for doxygen commands that should be mandatory (\brief, \param, \details ) to ensure interfaces are properly documented
There was a problem hiding this comment.
Hi @anand-ky see my comment above, we propose to have this in a separate document as it may not always be the same.
|
|
||
| In such a case (we usually use an expected duration of a few hundred milliseconds as the limit) or when the duration of an action is non deterministic and worse case would take longer than a few hundred milliseconds, it is advised to make the interface asynchronous. The interface will then only pass the data needed to start the action to the component and add a callback to the interface to make it possible for the implementation to return the result asynchronously. The component will handle the action in the background (do not use threads for this but use the Thunder workerpool) and call the provided callback when the action is completed to signal this to the initiator of the action. Not only will this prevent the call to fail because of JSON-RPC or COM-RPC timeouts but it will also make sure the framework is not blocked internally and also the client can continue without being blocked waiting for the result (or needed to reserve a thread for this). | ||
|
|
||
| For example an interface that will require user interaction for its action (like asking for the pin to be entered) is a perfect example of one that should be made asynchronous as it is of course completely nondeterministic when the call will return as the successful entering of the PIN could take minutes/hours potentially. |
There was a problem hiding this comment.
Can we add example of Async interface.
docs/plugin/interfaces/guidelines.md
Outdated
|
|
||
| In software development, this pattern is commonly used to establish communication between different parts of a system without them being directly coupled. The Publisher (or Subject) maintains a list of Subscribers (or Observers) interested in being notified of changes, and when an event occurs or state changes, the Publisher notifies all Subscribers by invoking specific methods on them. This allows for a loosely coupled architecture where changes in one component trigger updates in other components without them having explicit knowledge of each other. | ||
|
|
||
| As there is a one-to-many dependency the parent interface requires methods to add and remove the INotification. It is a guideline to call these ```core::hresult Register(INotifcation*)``` and ```core::hresult Unregister(const INotification*);``` |
There was a problem hiding this comment.
The last line sounds a bit incomplete.
docs/plugin/interfaces/guidelines.md
Outdated
|
|
||
| Return ```core::hresult``` on methods {#ReturnCorehresultOnMethods} | ||
| --------------------------------- | ||
| As most interfaces are designed to work over process boundaries, it is recommended to cater for issues during the passing of the process boundary, e.g. the other process might have crashed. This means that method calls on interfaces that seem triial but do change the state of an implementation, like ```SetTime()```, may fail due to cross process communication. If the return value of a method is defined as core::hresult, the Thunder COMRPC framework will signal issues with the communication by returning a negative value. The whole positive range from 0-2147483647 (0-0x7FFFFFFF) is free for the implementer to return values. |
There was a problem hiding this comment.
Strange rule and works only for COMRPC.
Also regarding the negative error codes, perhaps, there can be a reserved range (within the negative numbers) for framework specific error code, not the entire negative number range.
| |begin|end|Owner|ID storage| | ||
| |--|-|-|-| | ||
| |0x00000000|0x00000080|Thunder internal|```Thunder/Source/plugin/ids.h```| | ||
| |0x00000080|0x80000000|Public Plugin interfaces|```ThunderInterfaces/interfaces/ids.h```| |
There was a problem hiding this comment.
What exactly are public plugins and custom plugins?
| }; | ||
| ``` | ||
| At first this might seem logical but how would you expect the implementation to respond, if you would do Pause(), before Play()? Or Play() after Rewind() without stopping first? You would need to think about all the different options and specify how the interface should respond in that situation. | ||
| But if you think of it, what all the different methods do is specify a certain playback speed. So if you have one method with which you could set the player speed (and allow a negative number) it could do all the above while at the same time be consistent, easy to understand and no need to explain what will happen in certain situation as just the last set speed is the one that is current. |
There was a problem hiding this comment.
While I fully agree and endorse the "less is more" principle, often it is contextual on "how much is less". Clarity of the interface gets reduced after certain threshold on "less". In my opinion, we might want to make this sounds like a "follow the balanced approach".
| @@ -0,0 +1,383 @@ | |||
| Interface Definition | |||
| ==================== | |||
|
|
|||
There was a problem hiding this comment.
Adding a few generic comment.
First I really like what is already here.
- I think we might be missing some guidelines for how we recommend Interfaces are tagged (e.g @text:keep, @alt, ..) to properly support JsonRPC.
- Also, we might aligning on using or camel case vs Pascal case for methods, at the same time having it consistent across JsonRPC and Com-RPC
- A section with respect to versioning of interfaces may make sense. When an interface is extended, how is the version updated.
- Recommend we use enum class for types in interfaces
No description provided.