The rdkservices from rdkcentral

USE_DS definitions are not needed

Please remove USE_DS definitions. They are hard coded in platform.cmake files (not set in FindDS.cmake or CMakeLists.txt files depending on package availability). Building for any new platform I get errors even if having DS and find_package(DS).

https://github.com/search?q=repo%3Ardkcentral%2Frdkservices%20USE_DS&type=code

Revisit the WebKitBrowser implementation to avoid code duplication.

This is pending on completion of:
rdkcentral/Thunder#217
rdkcentral/Thunder#216

Move the logging functionality to the core:
https://github.com/rdkcentral/rdkservices/pull/162/files#diff-f733dc05f828d3910e543bf2e77f7961R265-R289
In the Portability.h -> DumpCallstack()
Leave the killing of this process up to the Thunder framework:
https://github.com/rdkcentral/rdkservices/pull/162/files#diff-f733dc05f828d3910e543bf2e77f7961R608-R614
Do this by calling IShell->Deactivate(reason => HANGING/WATCHDOG)

If we than update this, lets also remove the thread (saves resources again ;-) by turning this into a Job (https://github.com/rdkcentral/Thunder/blob/master/Source/core/WorkerPool.h#L35) and leave it up to the Timer (part of the workerPool) to trigger HangDetector.

The reason for these changes is because we need this functionality also in the Cobalt/Amazon and Netflix application and it is good to avoid code duplication :-)

CDM session remove logic should call Close instead of Run()

Session cleanup should happen via calling Close() and not using Run() with nullptr:

rdkservices/OpenCDMi/FrameworkRPC.cpp

Line 961 in 3d6928f

mediaKeySession->Run(nullptr);

Note: CDM implementations would need to adjust to this logic to remove the cb.

Lcov processing time has degraded

Generate coverage workflow step takes too long:
4m 16s
3m 15s
3m 32s

Need to exclude uninteresting sources from the processing, for example tests:

Processing Tests/CMakeFiles/RdkServicesTest.dir/tests/test_CountingLock.cpp.gcda
Processing Tests/CMakeFiles/RdkServicesTest.dir/tests/test_LocationSync.cpp.gcda
...

Container failures cause orphan rialto servers-RDKCOM-3921

There are two scenarios after rialto server is launched where container can fail, during intialization and during start of container. We need to ensure that rialtoserver is dismantled if that happens.

USE_IARMBUS/USE_IARM_BUS definitions are not needed

Please remove USE_IARM... definitions. They are hard coded in platform.cmake files (not set in FindIARMBus.cmake or CMakeLists.txt files depending on package availability). Building for any new platform I get errors even if having IARMBus and find_package(IARMBus).

https://github.com/search?q=repo%3Ardkcentral%2Frdkservices%20USE_IARM&type=code

JSON RPC interfaces should be in ThunderInterfaces repo

like it's done for ThunderNanoServices
e.g. https://github.com/rdkcentral/ThunderInterfaces/blob/master/jsonrpc/Power.json

interface is an agreement between provider and consumer of interface. it's not part of provider

Github workflow for unit tests

Unit testing (https://github.com/rdkcentral/rdkservices/tree/sprint/2203/RdkServicesTest) is manually triggered. Should automate it to track: the unit tests are building, passing, Valgrind reports no new findings. Indicate coverage in some way.

utils.cpp

Utils violate single responsibility:
https://github.com/rdkcentral/rdkservices/blob/sprint/2201/helpers/utils.h
https://github.com/rdkcentral/rdkservices/blob/sprint/2201/helpers/utils.cpp

and utils.cpp is built more than once (28 times):
https://github.com/rdkcentral/rdkservices/blob/sprint/2201/FrameRate/CMakeLists.txt#L27
https://github.com/rdkcentral/rdkservices/blob/sprint/2201/DataCapture/CMakeLists.txt#L27
...

To address these issues I've started replacing utils with single purpose headers like:
https://github.com/rdkcentral/rdkservices/blob/sprint/2203/helpers/UtilsIarm.h
https://github.com/rdkcentral/rdkservices/blob/sprint/2203/helpers/UtilsLogging.h

For example, DataCapture shared object size was reduced from 200036 to 109188 bytes.

There are still 37 rdkservices that use utils.h (28 build utils.cpp).

APIs take or support String for non-String data

Examples:

rdkservices/DisplaySettings/DisplaySettings.json

Lines 19 to 21 in b25e03e

    
           "summary": "Delay offset (in ms) on the selected audio port", 
        
           "type": "string", 
        
           "example": "0"

rdkservices/SystemServices/System.json

Lines 270 to 272 in b25e03e

    
           "summary": "The over temperature grace interval", 
        
           "type": "string", 
        
           "example": "600"

Functions getNumberParameter, getBoolParameter, etc:

rdkservices/helpers/UtilsJsonRpc.h

Lines 43 to 49 in b25e03e

    
           #define getNumberParameter(paramName, param) { \ 
        
               if (WPEFramework::Core::JSON::Variant::type::NUMBER == parameters[paramName].Content()) \ 
        
                   param = parameters[paramName].Number(); \ 
        
               else \ 
        
                   try { param = std::stoi( parameters[paramName].String()); } \ 
        
                   catch (...) { param = 0; } \ 
        
           }

encourage converting anything to int, bool, etc. This conflicts with schema which specifies a fixed type. Passing wrong data won't give an error. "bla bla" is interpreted as 0. "true" or "1" is true:

rdkservices/helpers/UtilsJsonRpc.h

Line 61 in b25e03e

    
           param = parameters[paramName].String() == "true" || parameters[paramName].String() == "1"; \

Another side effect is inadvertent try/catch enforce to build with -fexceptions:

rdkservices/SystemServices/CMakeLists.txt

Line 38 in b25e03e

    
           set_source_files_properties(SystemServices.cpp thermonitor.cpp platformcaps/platformcapsdata.cpp PROPERTIES COMPILE_FLAGS "-fexceptions")

Inter-plugin communication

Some RDK services use JSONRPC::LinkType for inter-plugin communication - DisplaySettings, MaintenanceManager, RDKShell, ScreenCapture, SystemServices, DeviceProvisioning, EasterEgg.

JSON-RPC is a web socket with a security token and JSON. It's used for JS-C++, or Shell-C++. Thunder exports JSONRPC::LinkType for C++ apps, however, some RDK services use it, even for inter-thread comm. in the same plugin:

rdkservices/RDKShell/RDKShell.cpp

Line 514 in d3bb5c7

    
           uint32_t status = thunderController->Invoke(RDKSHELL_THUNDER_TIMEOUT, api.c_str(), apiRequest.mRequest, joResult);

Plugin querying via an interface - IShell::QueryInterface (same process), or IShell::Root (same process or inter-process) - are better solutions for inter-plugin communication than a web socket connection with JSON.

Plugins should stop threads in Deinitialize

std::thread should be joined by a plugin before the destruction of a plugin. If that happens during the destruction of a plugin a thread can reference the members that have been destructed already and so segfault. I can see it is a common problem when people use ThreadRAII thinking it does everything for them. ThreadRAII as a member variable is not guaranteed to destroy before other member variables. Suggesting to remove ThreadRAII to avoid problems like that.

INTERNET / NETWORK preconditions

INTERNET is set by LocationSync and is used for example by WebKitBrowser/Amazon.
NETWORK is used by LocationSync.
Also, in some places like MaintenanceManager, org.rdk.Network.isConnectedToInternet is used instead of preconditions.
Similar is used in 0001-SERXIONE-600-LocationSync-Network-check-timer.patch by LocationSync.
I could not find what sets NETWORK and how it's different from org.rdk.Network.isConnectedToInternet.

servicemanager legacy code

"success" param in the api result is the legacy code.

In Thunder JSON-RPC there are error codes. If ERROR_NONE the message sends a "result" object. Otherwise the message sends an error code. Error codes are part of the interface:

rdkservices/Messenger/Messenger.json

Lines 76 to 77 in df8b598

    
           "description": "User name is already taken (i.e. the user has already joined the room)", 
        
           "$ref": "#/common/errors/illegalstate"

A caller shouldn't have to inspect "result" object to see if the api succeeded or not.

"getMeSomeData" methods are also the legacy code.

In Thunder JSON-RPC there are properties / readonly properties:

rdkservices/LocationSync/LocationSyncJsonRpc.cpp

Line 36 in df8b598

    
           Property<LocationData>(_T("location"), &LocationSync::get_location, nullptr, this);

WebKitBrowser: How to detect that url was successfully loaded or failed to load?

I tried to implement the way to detect that selected URL has been successfully loaded or failed to load and have problem with that.
There are following callbacks available in the browser plugin (WebKitImplementation.cpp file):

uriChangedCallback
loadChangedCallback
loadFailedCallback

*When a new domain URL load process is initiated, process seems to be straightforward:

url: https://www.live.bbctvapps.co.uk

uriChangedCallback (url: https://www.live.bbctvapps.co.uk)
loadChangedCallback() is being called with following events: WEBKIT_LOAD_STARTED, WEBKIT_LOAD_COMMITTED, WEBKIT_LOAD_FINISHED

URL load detection: wait for loadChangedCallback() with WEBKIT_LOAD_FINISHED event

*When loading url from the same domain (boot url has been already loaded), the only callback which is being called is uriChangedCallback():

boot url: https://widgets.metrological.com/lightning/liberty/2e3c4fc22f0d35e3eb7fdb47eb7d4658#boot
app url: https://widgets.metrological.com/lightning/liberty/2e3c4fc22f0d35e3eb7fdb47eb7d4658#app:com.metrological.app.Euronews

uriChangedCallback (url: https://widgets.metrological.com/lightning/liberty/2e3c4fc22f0d35e3eb7fdb47eb7d4658#app:com.metrological.app.Euronews)

URL load detection: wait for uriChangedCallback() with metrological url

*When loading wrong url from a new domain:

url: https://not_valid_url.com

uriChangedCallback (url: https://not_valid_url.com)
loadChangedCallback (event: WEBKIT_LOAD_STARTED)
uriChangedCallback (url: https://widgets.metrological.com/lightning/liberty/2e3c4fc22f0d35e3eb7fdb47eb7d4658#boot)
loadFailedCallback (url: https://widgets.metrological.com/lightning/liberty/2e3c4fc22f0d35e3eb7fdb47eb7d4658#boot)

URL failed detection: loadFailedCallback() contains previous url, but also there is uriChangedCallback() with metrological url which breaks failure detection

Could you please describe what is the proper way to handle success or failed url load detection?

need -I${includedir}/trower-base64/ to build

"trower-base64" is used without cmake find_ commands. target_link_libraries... trower-base64) works as the library is in /usr/lib. But #include "base64.h" does not work as the header is in /usr/include/trower-base64/base64.h. On yocto rdkservices build with a hack -I${includedir}/trower-base64/. The bug is in ContinueWatching and SystemAudioPlayer modules.

How to build?

How do you build the code in this repo? I haven't been able to find anything in the RDK wiki or the README although I may have missed something.

SystemServices constructor does some initialization as well

Plugins should initialize in IPlugin Initialize(). This is reflected in the repo readme.

SystemServices constructor makes a call to setMode which makes a call to IARM_Bus_Call. However, IARM is initialized in SystemServices::Initialize which is called after the constructor. SystemServices constructor relies on IARM being already initialized by other plugins. This isn't causing any issue but it might if someone configures SystemServices to run out of process.

This was found when writing unit tests for SystemServices.

Unit tests workflow should be fast when invoked locally

Developers need rapid feedback to safely and easily make incremental code changes. Disable (by default) coverage and Valgrind when invoked locally via ACT, to speed up the development of tests/ TDD. When invoked on GitHub, both should remain enabled.

Monitor lacks provision to read memory stat at the moment of query

Right now, Monitor plugin is implemented to read memory information from plugins at periodic intervals. But sometimes returning cached value are not desirable. If any component is interested in memory consumption of a plugin (say WebKitBrowser) at the moment, the only option we have is to increase the frequency of memory read.

Having a provision to force read memory stat from plugins would be of help in such scenarios.

Unit tests should use NiceMock

Unit tests should use NiceMock to address warnings like

| GMOCK WARNING:
| Uninteresting mock function call - taking default action specified at:
| /home/npoltorapavlo/dev/rdkservices/rdkservices/Tests/tests/test_Network.cpp:72:
|     Function call: IARM_Bus_RegisterEventHandler(0x7fe6bf416d87 pointing to "NET_SRV_MGR", 55, 0x7fe6bf3b7e42)
|           Returns: 0
| NOTE: You can safely ignore the above warning unless this call should not happen.  Do not suppress it by blindly adding an EXPECT_CALL() if you don't mean to enforce the call.  See https://github.com/google/googletest/blob/master/googlemock/docs/cook_book.md#knowing-when-to-expect for details.

Ignoring the Google Mock warning is unacceptable
NiceMock allows Google Mock to track interactions only for methods on which expectations exist.

Existing logs produce 3772 GMOCK WARNING and aren't readable.

LocationSync test hangs intermittently

[----------] 4 tests from LocationSyncTest
[ RUN      ] LocationSyncTest.activate_locationchange_location_deactivate
[       OK ] LocationSyncTest.activate_locationchange_location_deactivate (1152 ms)
[ RUN      ] LocationSyncTest.activate_sync_deactivate
Error: The operation was canceled.

org.rdk....

Observed the following issue.

If the plugin's callsign differs from the name, for example, due to "org.rdk." prefix:

# cat PersistentStore.json 
{
 "locator":"libWPEFrameworkPersistentStore.so",
 "classname":"PersistentStore",
 "precondition":[
  "Platform"
 ],
 "callsign":"org.rdk.PersistentStore",
 "autostart":true,
 "configuration":{
...

then configuration from the json file isn't loaded.

unit tests are failing

getting failure every time:

| [  FAILED  ] 2 tests, listed below:
| [  FAILED  ] NetworkTest.pingNamedEndpoint
| [  FAILED  ] NetworkTest.ping

someone added tests that use real network instead of mocks

detach() should not be used

When plugin deinits, the library is unloaded. Detached thread will be using address that is no longer in the address space of the application. Hence segfault. Crashes like this with empty stack trace are not possible to triage.

https://github.com/search?q=repo%3Ardkcentral%2Frdkservices%20detach()&type=code

onApplicationTerminated never fires

With the current implementation, by the time waitpid function returns in westeros compositor, we clear the listener for the corresponding compositor window. This result in never getting the WstClient_stoppedNormal or WstClient_stoppedAbnormal events to rdkcompositor.

As a backup strategy, we intent to send the event after we invoke the compositor kill call.

Unit tests build time has degraded

Building locally, rebuild after 1 small change in a test takes 3min. This eliminates the advantages of unit tests and should be fixed asap.

AbstractPlugin deprecation

AbstractPlugin isn't abstract. It's used to maintain the faulty versioning scheme pointed out in #2284. The issue was acknowledged and closed but not fixed. Readme still says "Plugins must extend the AbstractPlugin helper class." and "Removing or Adding an API should increment the version to the next whole number (2,3,4...).". The majority, 31 of 51 services use AbstractPlugin (7 of them really use versions other than 1). Among CPC services, it's 25 of 29 (only one uses versions other than 1).

Suggest removing AbstractPlugin.

Duplicated web-browser interfaces

This issue is to assess if potential rework is necessary around multiple web-browser interfaces with similar functionality.

IBrowserResources interface with UserScripts / UserStyleSheets methods was included into ThunderInterfaces: rdkcentral/ThunderInterfaces@cccdfaa

A year later a similar interface was also added to IBrowser.h: IBrowserScripting with RunJavaScript / AddUserScript / RemoveAllUserScripts methods:
rdkcentral/ThunderInterfaces@37e1fcf

The two interfaces overlaps in terms of functionality of executing user scripts but they are not exactly the same:

IBrowserResources::UserScripts executes scripts pointed by URI (file://)
IBrowserScripting::AddUserScript executes script code given as parameter

Please assess if the described functionalities scattered across multiple interfaces can be put in one place.
If I recall correctly any changes in Thunder interfaces can be introduced only when major version of these interfaces gets increased.

segfaults with "dbusCallHandler" signature

When Iarm event or call handlers are not unregistered and the plugin is unloaded, it can segfault with "dbusCallHandler" signature. Crashes like this with empty stack trace are not possible to triage.

Unit testing impediments and TDD

At present we have a number of developers who write tests for everything and usually facing the same problems explained further. Problem 1 (what to test). to write a test need to understand how the code works. plugins can be 5000+ lines of code, average 1000+. every line needs to be analyzed which takes time. need smaller testable modules, or "units". Problem 2 (who should do that). Developers who write tests are not familiar with this code and won't need it in future. Plugin owners can do a better job writing tests for their code. TDD looks like a better approach, as the tests are not added all at once, but along with the respective fix/feature. Problem 3 (how to build). the problem is external dependencies. getting a random plugin to build for tests can be a non-trivial work. This makes TDD complicated even to start. Maybe should change the approach a bit and focus on getting the plugins build. As a starting point for TDD can add one basic test which does nothing else but creating/destroying a plugin instance

Unit tests quality

A proper design of the tests extends their return and keeps them from becoming a maintenance burden.

Deriving highly descriptive test names is important. A test case name and test name together, left to right, should reveal a sentence that describes what is verified - TEST(ASet, IgnoresDuplicateAdded). When using fixtures, the fixture name should clearly describe the context - TEST_F(ASetWithOneItem, HasSizeOfOne).

Tests shouldn't require readers to slowly dig into each line of the test, trying to figure out what's going on. There should be 3 sections, separated by blank lines - setup (Arrange), execute code (Act), and verify that the behavior occurred as expected (Assert). It's known as Arrange-Act-Assert. Irrelevant details can be moved to the fixture or utility function.

A certain level of code coverage should never be the target. Programmers looking to bump up their code coverage numbers quickly figure out that they can write tests without assertions. These nontests are a waste of effort. A more sensible use of code coverage is to identify areas of code that needs more tests.

Tests that can fail for several reasons waste time to pinpoint the cause. A common mistake is to focus on member functions - TEST(ASet, Add) which results in a mix of different behaviors in a single test. One Assert per Test helps to have tests that fail for a specific reason, and drives to focus on one behavior at a time.

Unit means a small piece of logic independent from other parts. If coupled with other parts, it's integration testing. Integration tests are difficult to maintain and should be converted to fast unit tests by removing dependencies.

Handling of the unit tests external dependencies

Unit tests should address the rdk services. Unit tests should not address the extrenal code linked as libraries and included via include directive. I.E. such as DS (displaysettings), IARM, and others. At present, the https://github.com/rdkcentral/rdkservices/tree/sprint/22Q4/Tests/headers folder contains the fake headers used by the rdk services. The purpose of those fake headers is to make build possible and make external code mockable. It is observed that people prefer to copy-paste the original headers as is. As a result a considerable part of https://github.com/rdkcentral/rdkservices/tree/sprint/22Q4/Tests/headers folder contains the external code that serves none of the objectives. To avoid that, let's get rid of https://github.com/rdkcentral/rdkservices/tree/sprint/22Q4/Tests/headers folder. It makes sense to generate external headers as empty files by the workflow. The actual definitions (only those which are used) can be squeezed into files like ds.h, iarm.h, and others under the https://github.com/rdkcentral/rdkservices/tree/sprint/22Q4/Tests/mocks folder.

It should be easy to understand what unit test verifies

Tests have a common flow. A test sets up the conditions, then executes code, and finally verifies that the behavior occurred as expected.
Tests shouldn’t require readers to slowly dig into each line of the test, trying to figure out what’s going on. A reader should readily understand the essential elements of a test’s setup (arrange), what behavior it executes (act), and how that behavior gets verified (assert).
It's known as Arrange-Act-Assert or Setup-Execute-Verify, or Given-When-Then. In other words, Given a context, When the test executes some code, Then some behavior is verified.

TEST_F(ARetweetCollection, IgnoresDuplicateTweetAdded) {
    Tweet tweet("msg", "@user");
    Tweet duplicate(tweet);
    collection.add(tweet);

    collection.add(duplicate);

    ASSERT_THAT(collection.size(), Eq(1u));
}

Remove copy-paste patterns in unit tests

* impl; is declared in 23 classes. Move it to a template class e.g. IMockable.
... .impl = &...Mock; occurs in 100+ places. Move it to a mock constructor with IMockable param.
Almost every test case defines:

    Core::ProxyType<Plugin::...> plugin;
    Core::JSONRPC::Handler& handler;
    Core::JSONRPC::Connection connection;
    string response;

Move this to a template class and derive test cases from it.
FactoriesImplementation and PluginHost::IFactories::Assign occurs in 20 files. Move it to a class and derive test cases from it.
Etc.

use of shell subprocess

In most places there is no need to use system/popen. For example, it's used to:

modify a string:

rdkservices/Warehouse/Warehouse.cpp

Lines 656 to 657 in 537e566

    
           std::string script = "echo '" + path + "' | sed -r \"s/([^$]*)([$\\{]*)([^$\\{\\}\\/]*)(.*)/\\3/\""; 
        
           variable = Utils::cRunScript(script.c_str());

search a string in a file:

rdkservices/Warehouse/Warehouse.cpp

Lines 663 to 664 in 537e566

    
           script = ". /etc/device.properties; echo \"$" + variable + "\""; 
        
           value = Utils::cRunScript(script.c_str());

rdkservices/Network/Network.cpp

Lines 210 to 211 in 537e566

    
           const char * script1 = R"(grep DEVICE_TYPE /etc/device.properties | cut -d "=" -f2 | tr -d '\n')"; 
        
           m_isHybridDevice = Utils::cRunScript(script1).substr();

rdkservices/Warehouse/Warehouse.cpp

Line 545 in 537e566

FILE *p = popen("cat /proc/mounts | grep mmcblk0p1 | awk '{print $2}' ", "r");

search files:

rdkservices/Warehouse/Warehouse.cpp

Line 721 in 537e566

std::string result = Utils::cRunScript(script.c_str());

rdkservices/SystemServices/SystemServicesHelper.cpp

Lines 300 to 301 in 537e566

    
           command = "find "+path+" -iname "+filter+" > /tmp/tempBuffer.dat"; 
        
           retStat = system(command.c_str());

read a file:

rdkservices/SystemServices/SystemServices.cpp

Line 1413 in 537e566

string httpCodeStr = Utils::cRunScript("cat /tmp/xconf_httpcode_thunder.txt");

rdkservices/SystemServices/SystemServices.cpp

Line 1428 in 537e566

response = Utils::cRunScript("cat /tmp/xconf_response_thunder.txt");

rdkservices/SystemServices/SystemServices.cpp

Line 1908 in 537e566

FILE* fp = popen(CAT_DWNLDPROGRESSFILE_AND_GET_INFO, "r");

create a dir:

rdkservices/SystemServices/SystemServices.cpp

Lines 2193 to 2194 in 537e566

    
           std::string command = "mkdir -p " + dir + " \0"; 
        
           Utils::cRunScript(command.c_str());

remove a file:

rdkservices/SystemServices/SystemServices.cpp

Lines 2793 to 2794 in 537e566

    
           snprintf(cmd, 127, "rm -f %s", STANDBY_REASON_FILE); 
        
           pipe = popen(cmd, "r");

JSON stubs

Thunder's JsonGenerator can generate proxy stubs and JSON stubs. This practice is ignored in rdkservices. Only 12 of 52 plugins use JSON stubs.

rdkservices/LocationSync/LocationSync.h

Line 24 in 867aaec

#include <interfaces/json/JsonData_LocationSync.h>

40 of 52 use a bare JsonObject, and the names are written by hand.

README.md specifies that schemas are used "to generate API documentation as Markdown". However, schemas should also be used to generate JSON stubs, as it's done for the plugins in ThunderInterfaces.

Unit tests should describe the behavior

Deriving highly descriptive test names is important, as tests are read and reread by others who must maintain the code.
A common mistake is to focus on member functions TEST(ARetweetCollection, Add) which results in a mix of different behaviors in a single test.
Test behavior, not methods. A test case name and test name together, left to right, should reveal a sentence that describes what is verified:

TEST(ARetweetCollection, IgnoresDuplicateTweetAdded)
TEST(ARetweetCollection, UsesOriginalTweetTextWhenEmptyTweetAdded)
TEST(ARetweetCollection, ThrowsExceptionWhenUserNotValidForAddedTweet)

A list of test names should reflect the behaviors that the system supports.
When using fixtures, the fixture name should clearly describe the context:

TEST_F(ARetweetCollectionWithOneTweet, IsNoLongerEmpty)
TEST_F(ARetweetCollectionWithOneTweet, HasSizeOfOne)

Reboot, systemd and SIGTERM

Hi,

I have seen an issue related to the "reboot" request in System Services. An application on a STB is making this request after encountering a major error, in this case failing to launch the EPG (UI) app.

The application is started and managed by systemd. It expects to receive a SIGTERM signal to do a graceful shutdown. However, no SIGTERM is received by the application after the reboot request. Hence it is unable to do a graceful shutdown before reboot. This can sometimes results in a core dump file.

Can someone tell me if System Services does do a shutdown vis-à-vis systemd? It doesn't appear to be doing that. Otherwise I would expect to receive the SIGTERM signal from systemd.

Unit tests can use matcher assertions for the improved readability

The documentation: http://google.github.io/googletest/reference/matchers.html
The difference compared to regular assertions: https://en.wikipedia.org/wiki/Hamcrest
Matcher assertions read left to right:

using namespace ::testing;

ASSERT_THAT(actual, Eq("alpha"));

sleep()-s in code

aside from things like sleep(10); or sleep(60); the following can be often seen:

attempts to fix race conditions with sleep()

rdkservices/SystemServices/SystemServices.cpp

Lines 575 to 576 in 0ddc99f

    
           //give Netflix process some time to terminate gracefully. 
        
           sleep(10);

rdkservices/RDKShell/RDKShell.cpp

Lines 4989 to 4992 in 0ddc99f

    
           notify(RDKSHELL_EVENT_ON_WILL_DESTROY, params); 
        
           sleep(gWillDestroyEventWaitTime); 
        
           killAllApps();

rdkservices/DisplaySettings/DisplaySettings.cpp

Line 5044 in 0ddc99f

sleep(HDMICECSINK_PLUGIN_ACTIVATION_TIME);

rdkservices/DisplaySettings/DisplaySettings.cpp

Line 5064 in 0ddc99f

sleep(WARMING_UP_TIME_IN_SECONDS);

sleep() in a loop (what if reboot/Deinitialize occurs while looping?)

rdkservices/TextToSpeech/impl/TTSDownloader.cpp

Lines 76 to 78 in 0ddc99f

    
           while(tts_request.empty()) 
        
           { 
        
              sleep(60);

rdkservices/MaintenanceManager/MaintenanceManager.cpp

Lines 535 to 538 in 0ddc99f

    
           do{ 
        
               network_available = checkNetwork(); 
        
               if ( !network_available ){ 
        
                   sleep(30);

rdkservices/MaintenanceManager/MaintenanceManager.cpp

Lines 364 to 366 in 0ddc99f

    
           do{ 
        
               if ((getServiceState(m_service, "org.rdk.AuthService", state) != Core::ERROR_NONE) || (state != PluginHost::IShell::state::ACTIVATED)) { 
        
                   sleep(10);

sleep() in a loop with no retry count (what if condition never satisfies?)

rdkservices/HdmiCecSink/HdmiCecSink.cpp

Lines 2834 to 2837 in 0ddc99f

    
           while(m_currentArcRoutingState != ARC_STATE_ARC_TERMINATED)	 
        
                  { 
        
                         usleep(500000); 
        
                   }

unclear things like sleep() for 68 ms?

rdkservices/RDKShell/RDKShell.cpp

Lines 6450 to 6452 in 0ddc99f

    
           std::cout << "bounds set\n"; 
        
           usleep(68000); 
        
           std::cout << "all set\n";

Versioning

I think the versioning provided by Thunder:

https://github.com/rdkcentral/Thunder/blob/master/Source/plugins/JSONRPC.h#L116

is for the [rare] case when multiple variants of the same API should coexist. API version (uint8_t: 1, 2, etc) is not the same as the plugin version used for the documentation:

https://github.com/rdkcentral/rdkservices/blob/sprint/2201/FrameRate/FrameRatePlugin.json#L9

(In a similar fashion, when using SQLite library, version "3.38.0" is specified in the library documentation or changelog, but it's not specified when calling "sqlite3_open". If the library has multiple variants of the same API, it can be "sqlite3_open" or "sqlite3_open_v2" with additional parameters. "sqlite3_open_v2" doesn't mean it was added in SQLite "3.2.0", it means there are 2 variants to call this API)

I would expect the same logic in rdkservices but it's different:

For example, "org.rdk.FrameRate.2.setDisplayFrameRate" doesn't mean it differs from "org.rdk.FrameRate.1.setDisplayFrameRate". Instead, "org.rdk.FrameRate.1.setDisplayFrameRate" doesn't exist. On the other hand, "org.rdk.FrameRate.1.updateFps" and "org.rdk.FrameRate.2.updateFps" are the same. This looks like "2" = plugin version.

But in Thunder "2" is part of the method and not the callsign:

https://github.com/rdkcentral/Thunder/blob/master/Source/core/JSONRPC.h#L154

(This makes sense as no version is needed to activate the plugin)

However, the logic in AbstractPlugin:

https://github.com/rdkcentral/rdkservices/blob/sprint/2201/helpers/AbstractPlugin.h

makes it look like "2" = plugin version. This is confusing, because for the user, like an app, there's no difference. When app calls "org.rdk.FrameRate.1.setDisplayFrameRate" or "org.rdk.FrameRate.2.setDisplayFrameRate" and the plugin doesn't have it, the result in both cases will be an error (-32601 "Unknown method." or -32602 "Requested version is not supported."). There's no need for the app to specify that "setDisplayFrameRate" was added in "version 2", or "version 38" in order to just call it.

(As if SQLite adds a new API in version "3.38.1" and calls it "sqlite3_reopen_v3_38_1". At the same time, adds "sqlite3_open_v3_38_1" which doesn't differ from "sqlite3_open" and both can be called. Not sure if this makes sense, but such is the logic in AbstractPlugin)

Suggesting to remove AbstractPlugin and the logic it introduces.

need -I${includedir}/wdmp-c/ to build

rfcapi.h has #include <wdmp-c.h>. real path is include/wdmp-c/wdmp-c.h. When building rdkservices need to add -I${includedir}/wdmp-c/ as a workaround

How to play base64 encoded wav audio?

We use webkit to play audio, use html5 video tag "data:audio/x-wav;base64,", the platform can't support it.
So is there any other way to play it?

<platform>.cmake obstruct build

platform.cmake are not actively used.
Definitions are from old projects and not used in this project: USE_SOUND_PLAYER, ENABLE_DCS_BUILD_FLAGS, USE_DEVICE_SETTINGS_SERVICE, HAS_API_DEVICEPROVISIONING, ENABLE_SYSTEM_17 etc.

Some plugins are hard coded per platform: add_definitions (-DPLUGIN_WAREHOUSE). This makes us unable to turn them on/off from a bitbake recipe. Also dependencies for such plugins are not built.

USE_IARM* are hard coded per platform and conflict with find_package(IARMBus).

USE_DS are hard coded per platform and conflict with find_package(DS).

Flags are hard coded per platform: add_definitions (-DLOGUPLOAD_BEFORE_DEEPSLEEP) and can conflict with bitbake recipes: EXTRA_OECMAKE_append_skyxione-uk = " -DLOGUPLOAD_BEFORE_DEEPSLEEP=ON"

The code of AVInput

rdkservices/AVInput/AVInput.cpp

Line 205 in ffa2a2b

// Thunder plugins communication

For me, it's not clear what AVInput does.
It has 2 methods, which use device::HdmiInput. This is clear. However, the rest of the code:

starts a timer
creates a security token
checks if org.rdk.HdmiInput is activated
activates org.rdk.HdmiInput
listens to org.rdk.HdmiInput events over JSON-RPC

This is not clear as, at a glance, AVInput can operate without org.rdk.HdmiInput. If the authors wanted to get events from org.rdk.HdmiInput, there's an easier way - receive events from IARM directly.

Also, if org.rdk.HdmiInput doesn't exist, AVInput keeps trying to activate it indefinitely and the methods return failure "org.rdk.HdmiInput plugin is not ready". But my understanding is device::HdmiInput functions can be called without need in org.rdk.HdmiInput. There's also nothing in the documentation on why org.rdk.HdmiInput is needed.

I think "Thunder plugins communication" logic of AVInput should be removed. That is, lines 205-403.

Some tests are not working without valgrind

added a step to run without Valgrind and it's failing 93a9b5a
tests are expected to work regardless of Valgrind

PlayerInfo rdkservice has writable properties

Writable properties for "Info" services are against design goal that "Info" services are for obtaining information from system, not for modifying the behaviour of the system.

rdkservices/PlayerInfo/PlayerInfo.json

Line 214 in b98a056

"dolby_enableatmosoutput":{

rdkservices/PlayerInfo/PlayerInfo.json

Line 222 in b98a056

"dolby_mode":{

This changes were introduced with cleaning-up DeviceSettings documentation commit:
c05828b

helpers folder

/helpers is like a framework, but it's not a library/libraries. Files from it are just compiled in multiple places. For example, utils.cpp is compiled in 26 places (see issue #2319), SystemServicesHelper.cpp in 2 places, etc.

Also, most of the /helpers' functionality looks like reinventing the wheel. For example, Thunder gives us the logging TRACE, but /helpers "help" to avoid it and use LOGINFO. Utils::fileExists "help" to avoid Core::File(..).Exists(). 2 timer classes "help" to avoid Core::TimerType, etc.

Suggest using the functionality given by the Thunder framework and getting rid of /helpers.

unit tests without objective

rdkservices/Tests/tests/test_HdmiInput.cpp

Lines 187 to 191 in a1077d6

    
           TEST_F(HdmiInputDsTest, writeEDID) 
        
           { 
        
               EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("writeEDID"), _T("{\"deviceId\": 0, \"message\": \"message\"}"), response)); 
        
               EXPECT_EQ(response, string("{\"success\":true}")); 
        
           }

test passes and coverage good
but if writeEDID implementation is missing, also passes and coverage good

rdkservices/HdmiInput/HdmiInput.cpp

Lines 370 to 373 in a1077d6

    
           void HdmiInput::writeEDID(int deviceId, std::string message) 
        
           { 
        
           }

once writeEDID is implemented, test passes and coverage good
once bug appears in writeEDID, passes and good.

it is important to have clear and sufficient expectations

	"summary": "Delay offset (in ms) on the selected audio port",
	"type": "string",
	"example": "0"

	"summary": "The over temperature grace interval",
	"type": "string",
	"example": "600"

	#define getNumberParameter(paramName, param) { \
	if (WPEFramework::Core::JSON::Variant::type::NUMBER == parameters[paramName].Content()) \
	param = parameters[paramName].Number(); \
	else \
	try { param = std::stoi( parameters[paramName].String()); } \
	catch (...) { param = 0; } \
	}

	"description": "User name is already taken (i.e. the user has already joined the room)",
	"$ref": "#/common/errors/illegalstate"

	std::string script = "echo '" + path + "' \| sed -r \"s/([^$])([$\\{])([^$\\{\\}\\/])(.)/\\3/\"";
	variable = Utils::cRunScript(script.c_str());

	script = ". /etc/device.properties; echo \"$" + variable + "\"";
	value = Utils::cRunScript(script.c_str());

	const char * script1 = R"(grep DEVICE_TYPE /etc/device.properties \| cut -d "=" -f2 \| tr -d '\n')";
	m_isHybridDevice = Utils::cRunScript(script1).substr();

	command = "find "+path+" -iname "+filter+" > /tmp/tempBuffer.dat";
	retStat = system(command.c_str());

	std::string command = "mkdir -p " + dir + " \0";
	Utils::cRunScript(command.c_str());

	snprintf(cmd, 127, "rm -f %s", STANDBY_REASON_FILE);
	pipe = popen(cmd, "r");

	//give Netflix process some time to terminate gracefully.
	sleep(10);

	notify(RDKSHELL_EVENT_ON_WILL_DESTROY, params);
	sleep(gWillDestroyEventWaitTime);

	killAllApps();

	do{
	network_available = checkNetwork();
	if ( !network_available ){
	sleep(30);

	do{
	if ((getServiceState(m_service, "org.rdk.AuthService", state) != Core::ERROR_NONE) \|\| (state != PluginHost::IShell::state::ACTIVATED)) {
	sleep(10);

	while(m_currentArcRoutingState != ARC_STATE_ARC_TERMINATED)
	{
	usleep(500000);
	}

	std::cout << "bounds set\n";
	usleep(68000);
	std::cout << "all set\n";

	TEST_F(HdmiInputDsTest, writeEDID)
	{
	EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("writeEDID"), _T("{\"deviceId\": 0, \"message\": \"message\"}"), response));
	EXPECT_EQ(response, string("{\"success\":true}"));
	}

	void HdmiInput::writeEDID(int deviceId, std::string message)
	{

	}

rdkcentral / rdkservices Goto Github PK

rdkservices's People

Contributors

Stargazers

Watchers

Forkers

rdkservices's Issues

Recommend Projects

Recommend Topics

Recommend Org