Réglage de bit et lisibilité de code

Question

J'ai une application Arduino (en fait une bibliothèque) qui contient un certain nombre de drapeaux d'état - et à l'origine je les ai simplement déclarés comme ints (bien uint8_t donc 8 bits non signés). Mais j'aurais pu les combiner tous en un entier et utiliser les opérations de masque de bits pour définir et tester le statut.

Un exemple de l'ancien:

if (_shift == HIGH) 
{ 
    _shift = LOW; 
} 
else 
{ 
    _shift = HIGH; 
} 

un exemple de ce dernier

#define SHIFT_BIT 0 

if (bitRead(_flags, SHIFT_BIT) == HIGH) 
{ 
    bitWrite(_flags, SHIFT_BIT, LOW); 
} 
else 
{ 
    bitWrite(_flags, SHIFT_BIT, HIGH); 
} 

L'ancien lit mieux, mais celui-ci est plus efficace (l'espace et le temps). L'efficacité de l'espace et du temps devrait-elle toujours être gagnante dans cette situation ou s'agit-il d'une sorte d'optimisation qui ne devrait avoir lieu qu'en cas de besoin?

(Ajouté)

Pour être complet est ici la définition de câblage de ces bitWrite etc. macros:

#define bitRead(value, bit) (((value) >> (bit)) & 0x01) 
#define bitSet(value, bit) ((value) |= (1UL << (bit))) 
#define bitClear(value, bit) ((value) &= ~(1UL << (bit))) 
#define bitWrite(value, bit, bitvalue) (bitvalue ? bitSet(value, bit) : bitClear(value, bit)) 

Answer 1

Découvrez Raymond Chen's excellent take sur cette question. En résumé, vous devez effectuer un calcul détaillé pour savoir si le dernier cas est réellement plus efficace, en fonction du nombre d'objets par rapport au nombre d'objets qui définissent ces états.

En ce qui concerne la lisibilité, il semble que vous le fassiez avec des variables membres, ce qui signifie que vous les avez probablement encapsulées dans de jolies fonctions. Dans ce cas, je ne suis pas aussi concerné par la lisibilité, car au moins le code pour les personnes utilisant la classe est beau. Cependant, vous pouvez toujours l'encapsuler dans des fonctions privées si c'est un problème.

Answer 2

Il peut être plus clair si vous supprimez la nécessité d'utiliser les constantes HIGH et LOW, en les scindant en deux méthodes. Il suffit de faire les méthodes bitSet et bitClear. bitSet définit le bit sur HIGH et bitClear définit le bit sur LOW. Il devient alors:

#define SHIFT_BIT 0 

if (bitRead(_flags, SHIFT_BIT) == HIGH) 
{ 
    bitClear(_flags, SHIFT_BIT); 
} 
else 
{ 
    bitSet(_flags, SHIFT_BIT); 
} 

Et bien sûr, si vous venez de HIGH == 1 et LOW == 0, alors vous n'avez pas besoin du contrôle de ==.

Answer 3

Pour moi, même ce dernier code est encore assez lisible. En donnant un nom à chacun de vos drapeaux, le code peut être lu sans trop d'efforts.

Une mauvaise façon de le faire serait d'utiliser des « nombres magiques »:

if(_flags | 0x20) { // What does this number mean? 
    do_something(); 
} 

Answer 4

Pour les champs de bits, il est préférable d'utiliser des opérations logiques, de sorte que vous pouvez faire:

if (flags & FLAG_SHIFT) { 
    flags &= ~FLAG_SHIFT; 
} else { 
    flags |= FLAG_SHIFT; 
} 

Cette a maintenant le regard du premier avec l'efficacité de ce dernier. Maintenant, vous pourriez avoir des macros plutôt que des fonctions, donc (si j'ai ce droit - ce serait quelque chose comme):

#define bitIsSet(flags,bit) flags | bit 
#define bitSet(flags,bit) flags |= bit 
#define bitClear(flags,bit) flags &= ~bit 

Vous n'avez pas les frais généraux de l'appel de fonctions, et le code devient plus lisible encore.

Je n'ai pas encore joué avec Arduino, mais il y a peut-être déjà des macros pour ce genre de choses, je ne sais pas.

Answer 5

Selon la conformité du compilateur AVR-GCC, dont je ne suis pas sûr, vous pouvez faire quelque chose comme ça et garder les choses belles et propres.

struct flags { 
    unsigned int flag1 : 1; //1 sets the length of the field in bits 
    unsigned int flag2 : 4; 
}; 

flags data; 

data.flag1 = 0; 
data.flag2 = 12; 

if (data.flag1 == 1) 
{ 
    data.flag1 = 0; 
} 
else 
{ 
    data.flag1 = 1; 
} 

Si vous souhaitez également accès à l'ensemble drapeau int à la fois, puis:

union 
{ 
    struct { 
     unsigned int flag1 : 1; //1 sets the length of the field in bits 
     unsigned int flag2 : 4; 
    } bits; 
    unsigned int val; 
} flags; 

Vous pouvez ensuite accéder à un peu avec 2 niveaux de indirection: variable.bits.flag1 < --returns bit indicateur unique ou avec un seul niveau pour obtenir la valeur entière des drapeaux: < --returns int

Answer 6

Si vous n'avez pas besoin d'optimiser, ne le faites pas et utilisez la solution la plus simple.

Si vous avez besoin d'optimiser, vous devez savoir pour quoi:

• La première version serait peu plus rapide si vous ne définissez ou effacer le bit au lieu de basculer, parce que vous n'avez pas besoin de lire la mémoire.

• La première version est mieux w.r.t. concurrence Dans la seconde, vous avez read-modify-write, vous devez donc vous assurer que l'octet de mémoire n'est pas accédé simultanément. Habituellement, vous désactivez les interruptions, ce qui augmente quelque peu la latence de votre interruption. En outre, oublier de désactiver les interruptions peut conduire à des bogues très désagréables et difficiles à trouver (le plus méchant bug que j'ai rencontré jusqu'ici était exactement de ce genre).

• La première version est minimalement meilleure w.r.t. la taille du code (moins l'utilisation du flash), car chaque accès est une seule opération de chargement ou de stockage. La seconde approche nécessite des opérations de bits supplémentaires.

• La deuxième version utilise moins de RAM, surtout si vous avez beaucoup de ces bits.

• La deuxième version est également plus rapide si vous voulez tester plusieurs bits à la fois (par exemple, l'un des ensembles de bits).

Answer 7

Si vous parlez de lisibilité, bit-ensembles et C++, pourquoi ne pas trouver quoi que ce soit sur std::bitset là-dedans? Je comprends que la course du programmeur embarqué est assez confortable avec les masques bitmasks, et a développé une cécité pour sa laideur (les masques, pas les courses :), mais à part les masques et les bitfields, la bibliothèque standard a une solution assez élégante.

Un exemple:

#include <bitset> 

enum tFlags { c_firstflag, c_secondflag, c_NumberOfFlags }; 

... 

std::bitset<c_NumberOfFlags> bits; 

bits.set(c_firstflag); 
if(bits.test(c_secondflag)) { 
    bits.clear(); 
} 

// even has a pretty print function! 
std::cout << bits << std::endl;// does a "100101" representation. 

Answer 8

Je dirais que la première chose que je suis préoccupé par est: « #define SHIFT 0 » Pourquoi ne pas utiliser d'une constante plutôt qu'une macro? En ce qui concerne l'efficacité, la constante permet de déterminer le type, assurant ainsi qu'aucune conversion ne sera nécessaire par la suite. En ce qui concerne l'efficacité de votre technique: - d'abord, débarrassez-vous de la clause else (pourquoi mettre le bit à HIGH si sa valeur est déjà HIGH? - en second lieu, préférez avoir quelque chose de lisible en premier, les setters/getters inline utilisant le masquage de bits en interne ferait l'affaire, serait efficace et serait lisible. En ce qui concerne le stockage, pour C++, j'aurais tendance à utiliser un bitet (combiné avec une énumération).

Answer 9

Est-il trop simple juste pour dire:

flags ^= bit;