Discussion :

[Bktero]

Hello,

J'ai dans mon code une requête une structure Request avec plusieurs champs dont un qui s'appelle Type. En fonction de ce type, l'exécution de la requête utilise les autres champs de manière appropriée.

En lisant des trucs sur std::variant hier, j'ai fait un code d'essai très concluant. Le code complet donne ceci :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
#include <cassert>
#include <cstdint>
#include <variant>
 
struct ReadRequest {
	ReadRequest(std::uint8_t* data, std::size_t length) :
			data(data),
			length(length) {
		assert(data != nullptr);
	}
 
	std::uint8_t* const data;
	const std::size_t length;
};
 
struct WriteRequest {
	WriteRequest(std::uint8_t* data, std::size_t length) :
			data(data),
			length(length) {
		assert(data != nullptr);
	}
 
	const std::uint8_t* const data;
	const std::size_t length;
};
 
struct DoubleWriteRequest {
	DoubleWriteRequest(const std::uint8_t* data_first, std::size_t length_first,
			   const std::uint8_t* data_second, std::size_t length_second) :
			data_first(data_first),
			length_first(length_first),
			data_second(data_second),
			length_second(length_second) {
		assert(data_first != nullptr);
		assert(data_second != nullptr);
	}
 
	const std::uint8_t* const data_first;
	const std::size_t length_first;
	const std::uint8_t* const data_second;
	const std::size_t length_second;
};
 
using Request = std::variant<ReadRequest, WriteRequest, DoubleWriteRequest>;
 
//---------------------------------------------------
#include "Request.hpp"
 
extern "C" {
void spi_driver_read(std::uint8_t* data, std::size_t length);
void spi_driver_write(const std::uint8_t* data, std::size_t length);
}
 
class SpiBus {
public:
	void execute(const Request& request) {
		std::visit(requestProcessor_m, request);
	}
 
private:
	class RequestProcessor {
	public:
		void operator()(const ReadRequest& rr) {
			spi_driver_read(rr.data, rr.length);
		}
 
		void operator()(const WriteRequest& wr) {
			spi_driver_write(wr.data, wr.length);
		}
 
		void operator()(const DoubleWriteRequest& dwr) {
			spi_driver_write(dwr.data_first, dwr.length_first);
			spi_driver_write(dwr.data_first, dwr.length_second);
		}
	};
 
	RequestProcessor requestProcessor_m;
};
 
//---------------------------------------------------
#include <array>
#include "SpiBus.hpp"
 
int main() {
	SpiBus bus;
	std::array<std::uint8_t, 15> buffer;
 
	ReadRequest rr{buffer.data(), buffer.size()};
	bus.execute(rr);
 
//	ReadRequest bad_rr{nullptr, 42}; // triggers assert()
 
	WriteRequest wr{buffer.data(), buffer.size()};
	bus.execute(wr);
 
	DoubleWriteRequest dwr{buffer.data(), buffer.size(), buffer.data(), buffer.size()};
	bus.execute(dwr);
}

Il s'avère que ma structure Request actuelle (pas celle avec std::variant) possède un champ privé, un sémaphore, pour des besoins de synchronisation. Il est privé et accessible uniquement par la classe SpiBus qui est une amie. Pour avoir la même chose avec std::variant sans avoir à mettre ce champ dans toutes mes variantes, je me suis dit que je pourrais faire ça :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
using RequestVariant = std::variant<ReadRequest, WriteRequest, DoubleWriteRequest>;
 
struct Request : public RequestVariant {
private:
	int semaphore; // type bidon
};

Sauf que ça ne fait pas de chocapic.... J'ai dû rajouter des constructeurs :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
struct Request : public RequestVariant {
	Request(ReadRequest& request) :
			RequestVariant(request) {
	}
 
	Request(WriteRequest& request) :
			RequestVariant(request) {
	}
 
	Request(DoubleWriteRequest& request) :
			RequestVariant(request) {
	}
 
private:
	int semaphore; // type bidon
};

Et mon std::visit ne compile pas, avec comme pour tout template un peu avancé un message d'erreur à rallonge, mais qui je pense se résume à ça :

error: invalid use of incomplete type 'struct std::variant_size<Request>'
struct variant_size<const _Variant> : variant_size<_Variant> {};

Bref, comment feriez-vous ?

Merci d'avance

[Pyramidev]

Salut,

Mon message ne sera pas sur l'erreur de compilation, mais sur la conception.
Je crois que tu aimes un peu trop l'héritage public.

Le fait que Request dérive publiquement de RequestVariant a plusieurs conséquences, dont une qui est identique au cas où Request possèderait une variable membre publique de type RequestVariant : dans les deux cas, l'utilisateur de Request peut accéder directement au RequestVariant sous-jacent en ignorant le sémaphore.

Il y a aussi un autre problème : RequestVariant a un destructeur public non virtuel, donc n'a pas été conçu pour être une classe de base.

Est-ce que fusionner Request et SpiBus en une seule classe SpiBusRequest répondrait à ton besoin ?
SpiBusRequest aurait deux variables membres privées : un RequestVariant et un sémaphore.
SpiBusRequest aurait une méthode publique execute() qui viendrait de SpiBus::execute(const Request&).
Tu n'as probablement pas besoin de variable membre RequestProcessor. À la place, tu peux générer une instance de RequestProcessor directement dans la méthode qui appelle std::visit.

[Bktero]

Je me doutais qu'on allait surtout partir sur le terrain de la conception, je vais donc expliquer un peu plus le contexte.

Pour mieux contrôler l'utilisation du bus SPI (la ressource hardware donc), on a créé la classe SpiBus. Elle possède deux méthodes execute() et executeAndBlock() que les différents threads utilisent pour demander des lectures ou des écritures sur le bus, ces méthodes ainsi pour effet d'ajouter des Requests dans une file (FIFO). La première rend la main dés que la requête a été mise dans la file, la seconde jusqu'à ce que la requête ait été traitée. SpiBus possède aussi une méthode process() qui traite les requêtes présente dans la FIFO et c'est fonction est appelée régulièrement dans le contexte d'un unique thread.

Au début, on avait identifier 3 types de requêtes : READ, WRITE, WRITE_THEN_READ. Il y avait donc une structure Request telle que décrite précédemment. Sauf que maintenant, on se rend compte qu'on aurait peut-être besoin de gérer d'autres sortes de requêtes, notamment ce DOUBLE_WRITE (je vais pas rentrer dans les raisons techniques, le sujet n'est pas du tout là). std::variant me semblait une bonne solution pour gérer....des variantes. Ce n'est peut-être pas le cas en fait.

Ca ne me semble pas possible de fusionner SpiBus et Request puisque SpiBus possède une file de Request pour le moment. On envisage même d'avoir plusieurs files pour gérer des priorités. De plus, le besoin est vraiment d'avoir un sémaphore par requête. Le principe est que dans le cas où la executeAndBlock() est appelée, la fonction tente de prendre le sémaphore et se bloque ; quand process() traite la requête, il relâche le sémaphore et débloque ainsi le thread ayant appelé executeAndBlock(). C'est une mécanique classique de synchronisation de threads avec les RTOS embarqués.

J'ai fait de l'héritage publique parce que ça semblait pratique mais moi je cherche juste à rajouter mon sémaphore. J'ai fait un essai avec une classe BaseRequest qui contient le sémaphore et les variantes de Request en hérite. Ça fait le taf, mais ça pose quelque soucis lors du process() puisque du coup je me peux pas factoriser le traitement du sémaphore avant / après std::visit() puisqu'il faut que je récupère la variante courante pour accéder au sémaphore. Je vais essayer une approche avec de la composition.

PS : effectivement, l'instance RequestProcessor n'a pas besoin d'être une donnée membre. Mais comme il est state-less, il n'y a pas grand intérêt à le créer / supprimer à chaque fois que je visite non ?

[Bktero]

Je m'en suis sorti avec de la composition. Mes requêtes sont comme j'ai montré précédemment (j'ai juste rajouté une classe de base pour un champ commun dont je n'avais pas parlé et que les utilisateurs de la classe doivent renseigner). Je gère le sémaphore en interne de ma classe SpiBus :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
	struct RequestWithSemaphore {
		Request request;
		Semaphore* semaphore = nullptr;
	};
 
	void SpiBus:: executeAndBlock(const Request& request) {
		Semaphore semaphore;
		RequestWithSemaphore rws{request, &semaphore};
 
		// Normally done in process()
		std::visit(requestProcessor_m, rws.request);
		rws.semaphore->release();
 
		// Enf of execute()
		rws.semaphore->take();
	}

Si vous avez des commentaires supplémentaires je suis preneur mais je marque comme résolu puisque j'ai trouvé un contournement à mon problème (encore une fois j'ai vérifié l'adage "préférer la composition à l'héritage")

[Pyramidev]

Ca ne me semble pas possible de fusionner SpiBus et Request puisque SpiBus possède une file de Request pour le moment.

Ok. Du coup, c'est différent du SpiBus du message 1.

effectivement, l'instance RequestProcessor n'a pas besoin d'être une donnée membre. Mais comme il est state-less, il n'y a pas grand intérêt à le créer / supprimer à chaque fois que je visite non ?

Si RequestProcessor est une variable membre de classe alors, même s'il est sans état, il prend inutilement de la place en mémoire, sauf si on utilise l'attribut [[no_unique_address]], mais on ne l'aura qu'à partir du C++20.

Par contre, si tu l'instancie dans la fonction qui appelle std::visit, alors le compilateur devrait pouvoir optimiser et ne pas consommer de place. Le constructeur et le destructeur de RequestProcessor ne font rien, donc il n'y aura pas de coût de création ou de destruction.

[Bktero]

J'imaginais que le créer et le détruire à chaque fois allait faire monter et descendre la pile mais vu qu'il ne contient pas de données membres, il ne devrait rien consommer. Il y a sans doute plus de possibilité d'optimisation avec une variable temporaire. Je vais faire ça