WIP: Improvement/spir 1114/parameter structure #45

JBHarvey · 2019-02-15T14:58:27Z

!!! IMPORTANT!!! not final DO NOT MERGE YET

Created to minimize the review work

Introduit beaucoup de changements majeurs (en terme de comportement). Entre-autre :

Ajout de thread dans ServerCommunicator pour permettre de faire la reception async de Request
Ajout d'Assembler pour faire la logique, le traitement et les validations des requetes (mostly empty for now, but really helpful to clarify the code)
Ajout de nouvelles structures et chemins de produce/consume pour les Request/Response -> notament : RequestHandler
Ajout d'une nouvelle structure utilitaire tres utile et importante: Parameters -> Permet de batch les parametres et de les traiter facilement (mais la classe en elle meme est pas super belle)
Ajout d'un nouveau type: StringLiterals qui permet de creer des Strings a la compilation et de les passer en template parameters --> (ca c'est un brin de metaprog :success-kid: )
Introduction des std::tuple et fonctions utilitaires qui vont avec pour faciliter les listes de differents template parameters

… declared as a static type

… to have final logic linked and tested

…handling

…n error

…tor & RequestHandler

RphlBrdt · 2019-02-15T18:57:00Z

sensor-gateway/common/ConstantFunctionsDefinition.h

 #define SENSORGATEWAY_CONSTANTFUNCTIONSDEFINITION_H

-#include "ConstantValuesDefinition.h"
+#include "StaticTupleFunctions.h"


le nom est pas super parlant.

RphlBrdt · 2019-02-15T19:01:43Z

sensor-gateway/common/data-flow/RingBuffer.hpp

            auto currentPad = firstPad;
            RingBufferPad<T>* nextPad;
-            for (auto i = 1; i < RING_BUFFER_SIZE; ++i) {
+            for (size_t i = 1; i < RING_BUFFER_SIZE; ++i) {


Pourquoi tu laisses des auto partout et tu modifies celui la?

RphlBrdt · 2019-02-15T19:02:02Z

sensor-gateway/common/data-structure/ControlMessage.hpp

+        typedef Byte AWL;
+        typedef Byte GUARDIAN;
+    }
+


espace inutile

RphlBrdt · 2019-02-15T19:02:14Z

sensor-gateway/common/data-structure/ControlMessage.hpp

+}
+
+namespace DataFlow {
+


espace inutile

RphlBrdt · 2019-02-15T19:16:17Z

sensor-gateway/common/data-structure/parameter/ParameterConstants.hpp

+            using Text = std::string;
+        }
+        namespace Units {
+            // TODO: Use UCUM to correctly encode units


Laisser un TODO pour un code open source?

RphlBrdt · 2019-02-15T19:20:52Z

sensor-gateway/common/data-structure/parameter/Parameters.hpp

+        struct ExtractedType {
+            using type = T;
+        };
+


Espace en trop

RphlBrdt · 2019-02-15T19:21:18Z

sensor-gateway/common/data-structure/parameter/Parameters.hpp

+        public:
+
+            explicit Parameter()
+//                   : value(),


Pourquoi value() en commentaire

RphlBrdt · 2019-02-15T21:53:56Z

sensor-gateway/server-communication/ResponseWriter.hpp

+
+        using ServerCommunicator = SensorAccessLinkElement::ServerCommunicator<SERVER_STRUCTURES>;
+
+        using ThisClass = ResponseWriter<SENSOR_STRUCTURES, SERVER_STRUCTURES>;


ThisClass, Cela represente quoi ?

RphlBrdt · 2019-02-15T21:56:04Z

sensor-gateway/server-communication/ServerCommunicationPayloadTypes.hpp

+
+        namespace Details {
+
+            template<typename N>


Je suis plus trop a jours avec les templates mais typename N ( est ce que pour les template on s oblige a mettre des majuscules pour les typename ou est ce que c<est parce que tu as juste voulu mettre N? ) Par la suite tu mets quelques lignes plus loins typename ValueType

RphlBrdt · 2019-02-15T21:57:38Z

sensor-gateway/server-communication/ServerCommunicator.hpp

+            // TODO : Validate request is allowed --> throw if not
+            // TODO : Assemble Request
+            // TODO : Pass to RequestHandler
+//            for (auto messageIndex = 0; messageIndex < messages.size(); ++messageIndex) {


Ne pas laisser de TODO dans le master en open source je pense que cela pourrait etre une bonne chose?

RphlBrdt · 2019-02-15T22:07:29Z

test/manual-test/sensor-communication/KvaserCanCommunicationStrategyTest.cpp

            auto message = messages[j];
            std::fprintf(file,
                         "=================================================================================== \n");
            std::fprintf(file, "SENSOR_ID : %" PRIu64 "\n", message.id);


Je fais la remarque seulement a cet endroit mais pourquoi tu met pas using namespace std en haut ah lieu de devoir le reecrire a chaque fois?

RphlBrdt · 2019-02-18T06:02:01Z

sensor-gateway/common/StaticTupleFunctions.h

+                [&](auto... Is) { return f(std::get<Is>(t)...); }
+        );
+    }
+


espace inutile

RphlBrdt · 2019-02-18T06:04:15Z

test/test-suite/sensor-communication/SensorCommunicatorTest.cpp

        }
    }

+//    template<typename Communicator, typename Strategy>


En commentaire?

RphlBrdt · 2019-02-18T06:04:40Z

test/test-suite/server-communication/RequestHandlerTest.cpp

+    };
+}
+
+//TEST_F(RequestHandlerTest,


Attention test commentés

RphlBrdt · 2019-02-18T06:05:23Z

test/test-suite/server-communication/ResponseWriterTest.cpp

+
+namespace ResponseWriterTestMock {
+
+//    using RealisticDataStructures = Sensor::Test::RealisticImplementation::Structures;


JE suppose que c est en WIP d'ou les commentaires ?

RphlBrdt · 2019-02-18T06:07:03Z

test/utilities/mock/CatchingServerCommunicationStrategyMock.hpp

+        bool hasToReturnSpecificGetParameterValueContents;
+        GetParameterValueContentList getParameterValueContentsToReturn;
+
+        // TODO: Create a template class to inherit from when implementing this :


Attention comment

…eContent -> no bad request

Sebdevar

J'ai fait mon possible dans la review, mais l'implementation du gateway est beaucoup trop deep dans la technicalite du C++ pour ma comprehension.

Sebdevar · 2019-02-18T15:23:55Z

sensor-gateway/common/data-structure/parameter/ParameterConstants.hpp

+        }
+        namespace Units {
+            // TODO: Use UCUM to correctly encode units
+            using not_applicable = StringLiteral<decltype("n/a"_ToString)>;


Ce serais bon d'avoir aussi none = "". Ca eviterait d'avoir a gerer inutilement un n/a et de juste passer directement la valeur.

Sebdevar · 2019-02-18T15:28:26Z

sensor-gateway/common/data-structure/parameter/Parameters.hpp

+                typename GatewayParameterDefinition,
+                Byte SENSOR_INTERNAL_COMMAND,
+                Byte SENSOR_INTERNAL_ADDRESS,
+                uint8_t SENSOR_INTERNAL_TOTAL_LENGTH_IN_BITS,


sensor Internal total length in bits? Ca represente quoi?

Sebdevar · 2019-02-18T15:38:15Z

sensor-gateway/common/data-structure/parameter/Parameters.hpp

+                return nameTuple;
+            }
+
+            struct NameEquals {


Pourquoi une struct pour une egalite de nom... dont la seule utlite c'est de verifier l'egalite de deux string.... tu pourrait pas juste... valider l'egalite des strings la ou necessaire, ou au pire faire juste une fonction, pas besoin d'une struct pour ca.

pareil pour les autres cas semblables comme type name equals

sensor-gateway/common/data-structure/parameter/Parameters.hpp

Sebdevar · 2019-02-19T15:08:34Z

sensor-gateway/common/StringLiteralType.h

+    struct StringLiteral;
+
+    template<char... elements>
+    struct StringLiteral<StringType<elements...>> {


un template de template de templates o__O? C'est vraiment nécessaire ca?

Sebdevar · 2019-02-19T15:10:29Z

sensor-gateway/data-translation/AWLTranslationStrategy.h


        static inline DataFlow::Intensity const convertIntensityToSNR(DataFlow::Intensity const& intensity) noexcept {
-            DataFlow::Intensity const snr = ((intensity / 1024) / 2.0) - 21; // w+f
+            DataFlow::Intensity const snr = (intensity / 2.0) - 21; // w+f


Sebdevar · 2019-02-19T15:49:12Z

sensor-gateway/server-communication/RequestHandler.hpp

+                responseWriter(responseWriter),
+                dataTranslator(dataTranslator) {}
+
+        ~RequestHandler() noexcept {};


Suggested change

~RequestHandler() noexcept {};

~RequestHandler() noexcept override = default;

Sebdevar · 2019-02-19T15:51:27Z

sensor-gateway/server-communication/RequestHandlingErrorFactory.h

+        Category category = COMMUNICATION_ERROR;
+        Severity severity;
+        std::string errorMessage;
+        if (errorCode == BAD_REQUEST) {


c'est le troisieme if else que je vois pour le error handling... pourquoi tu utilise pas un switch case? c'est beaucoup plus clair a la lecture.

https://www.geeksforgeeks.org/switch-vs-else/

Va voir la section 6, c'est la base de trucs a faire / eviter dans le gateway. Certains ne sont pas respectes pour de bonnes raisons, mais on essaie de les suivres pour la plupart

es-tu sure de l'avoir bien lu, la switch est valide et compliant dans ce que tu m'as link.

sensor-gateway/server-communication/ResponseWriter.hpp

…nicator, complete ServerCommunicator to RequestHandler code + tests

…egy + tests

Sebdevar · 2019-03-01T16:30:28Z

sensor-gateway/common/error/ErrorConstants.h

        std::string const SEPARATOR = " - ";
        std::string const EMPTY_MESSAGE = "";
+
+        std::string const PARAMETER_NOT_AVAILABLE = "Parameter is not available";


Clang-Tidy: Initialization of 'PARAMETER_NOT_AVAILABLE' with static storage duration may throw an exception that cannot be caught

Warning Message for all const created in this file.

Sebdevar · 2019-03-01T16:32:50Z

sensor-gateway/parameter-control/SensorParameterController.hpp

+                dataTranslator(dataTranslator),
+                responseControlMessageScheduler(this) {}
+
+        ~SensorParameterController() noexcept {


Suggested change

~SensorParameterController() noexcept {

~SensorParameterController() noexcept override {

Sebdevar · 2019-03-01T16:33:25Z

sensor-gateway/parameter-control/SensorParameterController.hpp

+
+    public:
+
+        explicit SensorParameterController(RequestHandler* requestHandler,


Never used.

Sebdevar · 2019-03-01T16:34:28Z

sensor-gateway/parameter-control/SensorParameterController.hpp

+        DataTranslator* dataTranslator;
+
+        SensorControlMessages sensorControlMessageRequests;
+        SensorControlMessagePointers sensorControlMessageRequestPointers;


Both fields never used.

Sebdevar · 2019-03-01T16:37:30Z

sensor-gateway/server-communication/ServerCommunicationPayloadTypes.hpp

+        namespace Details {
+
+            template<typename N, typename ValueType, ValueType v>
+            struct NameValuePayload {


Never Used.

Sebdevar · 2019-03-01T18:00:41Z

sensor-gateway/server-communication/ServerCommunicationPayloadTypes.hpp

+
+        public:
+
+            explicit StringPayload(std::string const& value) noexcept : value(value) {}


Warning: Clang-Tidy: Pass by value and use std::move

Not sure how to fix, I tried following this link

Sebdevar · 2019-03-01T18:01:23Z

sensor-gateway/server-communication/ServerCommunicationPayloadTypes.hpp

+                    return value;
+                }
+
+                Type const& getValue() const noexcept {


Never used.

Sebdevar · 2019-03-01T18:01:26Z

sensor-gateway/server-communication/ServerCommunicationPayloadTypes.hpp

+                    return value;
+                }
+
+                static constexpr bool isSuccess() noexcept {


Never used.

Sebdevar · 2019-03-01T18:09:41Z

sensor-gateway/server-communication/ServerCommunicationStrategy.hpp

+//
+//        virtual void sendResponse(RealNumberParameterResponse&& realNumberParameterResponse) = 0;
+//
+//        virtual void sendResponse(BooleanParameterResponse&& booleanParameterResponse) = 0;


commented code?

Sebdevar · 2019-03-01T18:12:12Z

sensor-gateway/server-communication/ServerRequest.hpp

+
+    public:
+
+        explicit ServerRequest(PayloadType const& payload) noexcept :


badRequest et CausedAnError sont pas initialises dans le constructor

Sebdevar · 2019-03-01T18:12:38Z

sensor-gateway/server-communication/ServerRequest.hpp

+            return badRequest;
+        }
+
+        bool const& hasCausedAnError() const noexcept {


JBHarvey added 16 commits December 13, 2018 16:53

Attempt at fixing the new intensity computation

37e12c8

Add base metaprog sturcture for parameter handling

bf86889

Cleanup ParametersTest

ed4bb80

Update and correct faulty test from unit transformation

1263623

Add ControlMessage code for Get, BlkGet, Set & BlkSet

3afc0d3

Add base for SensorRequest to unpack ServerRequests

c10b1f7

Add Working base to create SensorLists

5bf930a

Prepares to add final Parameters integration test with GatewayStructures

8ed2b10

Add test that validates a heterogenous variable parameter list can be…

da3f41d

… declared as a static type

Create core base for working ControlMessage in SensorAccessLink, need…

c926564

… to have final logic linked and tested

Acheive a start of validation for RequestHandling

b1261e2

Add validation that name is within availble parameters

4ae0dc0

Refactor and cleanup ServerCommunicationStrategies

b1ec323

Add threaded behavior to ServerCommunicator for asynchronous Request …

0bf6ae8

…handling

Add handling error production if ServerCommunicationStrategy throws a…

d3699c4

…n error

Remove circular dependency and prepare for linking of ServerCommunica…

d58ffc3

…tor & RequestHandler

RphlBrdt reviewed Feb 15, 2019

View reviewed changes

sensor-gateway/common/data-structure/ControlMessage.hpp

typedef Byte AWL;

typedef Byte GUARDIAN;

}

Copy link

Contributor

RphlBrdt Feb 15, 2019

Choose a reason for hiding this comment

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

espace inutile

RphlBrdt reviewed Feb 15, 2019

View reviewed changes

sensor-gateway/common/data-structure/ControlMessage.hpp

}

namespace DataFlow {

Copy link

Contributor

RphlBrdt Feb 15, 2019

Choose a reason for hiding this comment

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

espace inutile

RphlBrdt reviewed Feb 15, 2019

View reviewed changes

sensor-gateway/common/data-structure/parameter/Parameters.hpp

struct ExtractedType {

using type = T;

};

Copy link

Contributor

RphlBrdt Feb 15, 2019

Choose a reason for hiding this comment

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

Espace en trop

RphlBrdt reviewed Feb 15, 2019

View reviewed changes

RphlBrdt reviewed Feb 18, 2019

View reviewed changes

sensor-gateway/common/StaticTupleFunctions.h

[&](auto... Is) { return f(std::get<Is>(t)...); }

);

}

Copy link

Contributor

RphlBrdt Feb 18, 2019

Choose a reason for hiding this comment

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

espace inutile

RphlBrdt reviewed Feb 18, 2019

View reviewed changes

JBHarvey added 2 commits February 18, 2019 13:33

Create complete base calls from strategy->Assembler->RequestHandler

016d262

Complete tests for multiple ServerCommunicator->fetchGetParameterValu…

772074e

…eContent -> no bad request

Sebdevar reviewed Feb 19, 2019

View reviewed changes

JBHarvey added 10 commits February 19, 2019 12:44

Restructure RequestHandler + SensorParameterController w/ ServerCommu…

ce246b4

…nicator, complete ServerCommunicator to RequestHandler code + tests

Rename Is in templates for Indices

02d1afa

Rename Is in templates for Indices

b67bde3

Create base error Response and Assembler functions

fd6fbdc

Create Response types for GetParameterValue

512d699

Create stable assembler to create error response

6c167ac

Add Behavior + test for sending response to Strategy

5081681

Add Send Parameter Error ServerCommunicator->ServerCommunicationStrat…

8c400ed

…egy + tests

Complete first integration test for Erronous name in parameter request

0726368

Complete base for reusable and quickly writable mocks

7e2956f

Sebdevar reviewed Mar 1, 2019

View reviewed changes


		using ServerCommunicator = SensorAccessLinkElement::ServerCommunicator<SERVER_STRUCTURES>;

		using ThisClass = ResponseWriter<SENSOR_STRUCTURES, SERVER_STRUCTURES>;


		namespace ResponseWriterTestMock {

		// using RealisticDataStructures = Sensor::Test::RealisticImplementation::Structures;

	~RequestHandler() noexcept {};
	~RequestHandler() noexcept override = default;

	~SensorParameterController() noexcept {
	~SensorParameterController() noexcept override {


		public:

		explicit SensorParameterController(RequestHandler* requestHandler,


		public:

		explicit StringPayload(std::string const& value) noexcept : value(value) {}


		public:

		explicit ServerRequest(PayloadType const& payload) noexcept :

WIP: Improvement/spir 1114/parameter structure #45

Are you sure you want to change the base?

WIP: Improvement/spir 1114/parameter structure #45

Uh oh!

Conversation

JBHarvey commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

!!! IMPORTANT!!! not final DO NOT MERGE YET

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sebdevar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

JBHarvey commented Feb 15, 2019 •

edited

Loading