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.

Sign up for GitHub

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

Jump to bottom

✨ (rc): Start BLE + Start Battery Service #508

Merged
merged 2 commits into from
Mar 10, 2022
Merged

✨ (rc): Start BLE + Start Battery Service #508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cce66fb
:memo: (copyright): Add missing copyright
YannLocatelli Feb 28, 2022
f6b5006
:sparkles: (rc): Add BLEKit
YannLocatelli Feb 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/os/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ auto main() -> int
auto hello = HelloWorld();
hello.start();

rc.initializeComponents();
rc.registerEvents();

while (true) {
Expand Down
2 changes: 1 addition & 1 deletion libs/RobotKit/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ target_sources(RobotKit
)

target_link_libraries(RobotKit
PRIVATE
BLEKit
)

if (${CMAKE_PROJECT_NAME} STREQUAL "LekaOSUnitTests")
Expand Down
7 changes: 6 additions & 1 deletion libs/RobotKit/include/RobotController.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#pragma once

#include "BLEKit.h"

#include "StateMachine.h"
#include "interface/RobotController.h"
#include "interface/drivers/Battery.h"
Expand All @@ -27,11 +29,12 @@ class RobotController : public interface::RobotController

void raise(auto event) { state_machine.process_event(event); };

void initializeComponents() { _ble.init(); }
Copy link
Member

Choose a reason for hiding this comment

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

est-ce qu'on peut pas le faire dans le constructeur? plutôt que d'appeler une fonction qu'on peut oublier d'appeler ? ou alors le ctor appelle la fonction privée initializeComponents.

je me pose la même question pour registerEvents

Copy link
Member Author

@YannLocatelli YannLocatelli Mar 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

On a déjà eu dans le passé des cas où le constructeur qui devait appeler des méthodes privées ne le faisait pas à l'exécution. On peut reprendre le pari d'essayer.

Copy link
Member

Choose a reason for hiding this comment

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

On a déjà eu dans le passé des cas où le constructeur qui devait appeler des méthodes privées ne le faisait pas à l'exécution

on se souvient desquels et de pourquoi ça marchait pas?

ça serait intéressant de comprendre pourquoi, s'il fallait modifier un truc de notre côté ou si c'est "comme ça".

si pas le choix, on vivra avec. mais si c'est possible, ça rend l'interface et l'utilisation beaucoup plus simple et propre.

Copy link
Member Author

Choose a reason for hiding this comment

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

Je vais essayer de reproduire le phénomène

On avait pris le parti d'être explicite et d'appeler les méthodes qui fallait plutôt qu'implicite comme pourrait l'être un nom de méthode générique init();

Il peut y avoir une réponse du côté de most vexing parse [Source 1] [Source 2]

Copy link
Member Author

@YannLocatelli YannLocatelli Mar 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Après, c'est aussi pour les tests: c'est plus simple de focaliser sur ce qu'on attend de chaque fonctions au lieu d'avoir tous les appels et de devoir faire un EXPECT_CALL pour les unexpected calls

On peut les garder sous forme de fonctions, mais ça peut prêter à confusion d'avoir des fonctions publique qu'on appelle pas dans l'exécution du programme et uniquement en test

Edit. En fait c'est aussi le cas pour toutes les autres méthodes de RobotController, des méthodes qui ne sont appelées que par les tests et pas dans le main.cpp. Donc ça ne gênera pas d'avoir des méthodes publiques non appelé dans main.cpp

Copy link
Member Author

@YannLocatelli YannLocatelli Mar 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Aussi, il semblerait que les tests soient aussi cassés

Même si les EXPECT_CALL sont définis, ils ne sont pas bien pris en compte à l'exécution des tests et on se retrouve avec 2 types d'erreurs: d'un côté il y a des uninterresting calls et de l'autre des EXPECTED_CALL qui ne sont pas appelés.

Je vais voir s'il y a quelque chose à faire, dans ce cas je supprimerai ce commentaire

Edit. J'ai une solution, mais elle n'est pas très "propre" puisqu'elle impose 2 lignes supplémentaires dans chaque test.


void registerEvents()
{
using namespace system::robot::sm;

// Initializations
// Setup callbacks for each events

auto on_sleep_timeout = [this]() { raise(event::sleep_timeout_did_end {}); };
Expand All @@ -51,6 +54,8 @@ class RobotController : public interface::RobotController
interface::Timeout &_sleep_timeout;

interface::Battery &_battery;

BLEKit _ble;
};

} // namespace leka
4 changes: 4 additions & 0 deletions libs/RobotKit/source/RobotController.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,5 @@
// Leka - LekaOS
// Copyright 2022 APF France handicap
// SPDX-License-Identifier: Apache-2.0

#include "RobotController.h"
17 changes: 16 additions & 1 deletion libs/RobotKit/tests/RobotController_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#include "RobotController.h"

#include "ble_mocks.h"

#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include "mocks/leka/Battery.h"
Expand All @@ -27,14 +29,16 @@ class RobotControllerTest : public testing::Test
protected:
void SetUp() override
{
ble::init_mocks();

EXPECT_CALL(sleep_timeout, onTimeout).WillOnce(GetCallback<interface::Timeout::callback_t>(&on_sleep_timeout));
EXPECT_CALL(battery, onChargeDidStart).WillOnce(GetCallback<mbed::Callback<void()>>(&on_charge_did_start));
EXPECT_CALL(battery, onChargeDidStop).WillOnce(GetCallback<mbed::Callback<void()>>(&on_charge_did_stop));
EXPECT_CALL(sleep_timeout, start).Times(1); // Hide Uninteresting mock function call

rc.registerEvents();
}
// void TearDown() override {}
void TearDown() override { ble::delete_mocks(); }

mock::Timeout sleep_timeout {};
mock::Battery battery {};
Expand All @@ -52,6 +56,17 @@ TEST_F(RobotControllerTest, initialization)
EXPECT_NE(&rc, nullptr);
}

TEST_F(RobotControllerTest, initializeComponents)
{
ble::GapMock &mbed_mock_gap = ble::gap_mock();
ble::GattServerMock &mbed_mock_gatt = ble::gatt_server_mock();

EXPECT_CALL(mbed_mock_gap, setEventHandler).Times(1);
EXPECT_CALL(mbed_mock_gatt, setEventHandler).Times(1);

rc.initializeComponents();
}

TEST_F(RobotControllerTest, stateSetupEventSetupComplete)
{
rc.state_machine.set_current_states(lksm::state::setup);
Expand Down