##Not much left to review
Not much left to review
Your static totallength and encodedlen looks like helpers that should in my opinion be private, have a bit different names (already pointed out in other review) and there could possibly be non-static public members taking no parametr for this.
##A bit about the design, you have mentioned:
A bit about the design, you have mentioned:
- the underlying microprocessor may be 16-bit, or 32-bit or 64-bit
- the underlying microprocessor may be little-endian or big-endian
How are you going to solve that? What I am missing is some operator << (or push_back or append) that would be used for building the packet. That could requiere a whole different class if you really need to minimize the total memory footprint, but I would not care for one more int or size_t member called e.g. position.
Packet& operator << (unsigned v) {
reserve(4);
_data[position++] = v >> 24;
_data[position++] = v >> 16;
_data[position++] = v >> 8;
_data[position++] = v;
}
Note: Maybe I did not get it right and BER is not designed for complex packets, but for simple tagged values. In that case, all could be done in constructor.
Here you can see I have renamed your class from Ber to Packet, while using some reserve method that would handle possible reallocation. The data is always encoded as big-endian (and packed), independent on the MCU architecture. (Note: Some prefer to use data_ instead of _data, but the later is allowed within classes, forbidden in global space).
The same could be done for reading, possibly creating two other classes: PacketReader and PacketWriter, but you can have it both embedded in the Packet (or class Ber).
The way you have it now, you have to create all the data at once (while handling the big/little endian) and pass to constructor:
Ber(uint8_t tag, unsigned short length=0, const uint8_t *message=nullptr)
What I am proposing is to have it this way:
auto pkt = Packet(MSG_TAG) << (byte)data1 << (word)data2 << (uint)data3;
auto pkt2 = Packet(MSG_TAG, 1+2+4) << ....the same as above - space reserved ahead
auto pkt3 = Packet::construct(MSG_TAG, (byte)data1, ....);
Much easier to work with (the last one needs variadic templates, may not be accessible in your compiler).
##Finally, my proposals for naming:
Finally, my proposals for naming:
Ber -> Packet
totallength -> total_length or full_length
msglength -> length (most used I think) or payload_length / data_length or message_length
lencode -> encode_length
encodedlen -> length_size (well, this one is hard to properly name)
##Not much left to review
Your static totallength and encodedlen looks like helpers that should in my opinion be private, have a bit different names (already pointed out in other review) and there could possibly be non-static public members taking no parametr for this.
##A bit about the design, you have mentioned:
- the underlying microprocessor may be 16-bit, or 32-bit or 64-bit
- the underlying microprocessor may be little-endian or big-endian
How are you going to solve that? What I am missing is some operator << (or push_back or append) that would be used for building the packet. That could requiere a whole different class if you really need to minimize the total memory footprint, but I would not care for one more int or size_t member called e.g. position.
Packet& operator << (unsigned v) {
reserve(4);
_data[position++] = v >> 24;
_data[position++] = v >> 16;
_data[position++] = v >> 8;
_data[position++] = v;
}
Note: Maybe I did not get it right and BER is not designed for complex packets, but for simple tagged values. In that case, all could be done in constructor.
Here you can see I have renamed your class from Ber to Packet, while using some reserve method that would handle possible reallocation. The data is always encoded as big-endian (and packed), independent on the MCU architecture. (Note: Some prefer to use data_ instead of _data, but the later is allowed within classes, forbidden in global space).
The same could be done for reading, possibly creating two other classes: PacketReader and PacketWriter, but you can have it both embedded in the Packet (or class Ber).
The way you have it now, you have to create all the data at once (while handling the big/little endian) and pass to constructor:
Ber(uint8_t tag, unsigned short length=0, const uint8_t *message=nullptr)
What I am proposing is to have it this way:
auto pkt = Packet(MSG_TAG) << (byte)data1 << (word)data2 << (uint)data3;
auto pkt2 = Packet(MSG_TAG, 1+2+4) << ....the same as above - space reserved ahead
auto pkt3 = Packet::construct(MSG_TAG, (byte)data1, ....);
Much easier to work with (the last one needs variadic templates, may not be accessible in your compiler).
##Finally, my proposals for naming:
Ber -> Packet
totallength -> total_length or full_length
msglength -> length (most used I think) or payload_length / data_length or message_length
lencode -> encode_length
encodedlen -> length_size (well, this one is hard to properly name)
Not much left to review
Your static totallength and encodedlen looks like helpers that should in my opinion be private, have a bit different names (already pointed out in other review) and there could possibly be non-static public members taking no parametr for this.
A bit about the design, you have mentioned:
- the underlying microprocessor may be 16-bit, or 32-bit or 64-bit
- the underlying microprocessor may be little-endian or big-endian
How are you going to solve that? What I am missing is some operator << (or push_back or append) that would be used for building the packet. That could requiere a whole different class if you really need to minimize the total memory footprint, but I would not care for one more int or size_t member called e.g. position.
Packet& operator << (unsigned v) {
reserve(4);
_data[position++] = v >> 24;
_data[position++] = v >> 16;
_data[position++] = v >> 8;
_data[position++] = v;
}
Note: Maybe I did not get it right and BER is not designed for complex packets, but for simple tagged values. In that case, all could be done in constructor.
Here you can see I have renamed your class from Ber to Packet, while using some reserve method that would handle possible reallocation. The data is always encoded as big-endian (and packed), independent on the MCU architecture. (Note: Some prefer to use data_ instead of _data, but the later is allowed within classes, forbidden in global space).
The same could be done for reading, possibly creating two other classes: PacketReader and PacketWriter, but you can have it both embedded in the Packet (or class Ber).
The way you have it now, you have to create all the data at once (while handling the big/little endian) and pass to constructor:
Ber(uint8_t tag, unsigned short length=0, const uint8_t *message=nullptr)
What I am proposing is to have it this way:
auto pkt = Packet(MSG_TAG) << (byte)data1 << (word)data2 << (uint)data3;
auto pkt2 = Packet(MSG_TAG, 1+2+4) << ....the same as above - space reserved ahead
auto pkt3 = Packet::construct(MSG_TAG, (byte)data1, ....);
Much easier to work with (the last one needs variadic templates, may not be accessible in your compiler).
Finally, my proposals for naming:
Ber -> Packet
totallength -> total_length or full_length
msglength -> length (most used I think) or payload_length / data_length or message_length
lencode -> encode_length
encodedlen -> length_size (well, this one is hard to properly name)
##Not much left to review
Your static totallength and encodedlen looks like helpers that should in my opinion be private, have a bit different names (already pointed out in other review) and there could possibly be non-static public members taking no parametr for this.
##A bit about the design, you have mentioned:
- the underlying microprocessor may be 16-bit, or 32-bit or 64-bit
- the underlying microprocessor may be little-endian or big-endian
How are you going to solve that? What I am missing is some operator << (or push_back or append) that would be used for building the packet. That could requiere a whole different class if you really need to minimize the total memory footprint, but I would not care for one more int or size_t member called e.g. position.
Packet& operator << (unsigned v) {
reserve(4);
_data[position++] = v >> 24;
_data[position++] = v >> 16;
_data[position++] = v >> 8;
_data[position++] = v;
}
Note: Maybe I did not get it right and BER is not designed for complex packets, but for simple tagged values. In that case, all could be done in constructor.
Here you can see I have renamed your class from Ber to Packet, while using some reserve method that would handle possible reallocation. The data is always encoded as big-endian (and packed), independent on the MCU architecture. (Note: Some prefer to use data_ instead of _data, but the later is allowed within classes, forbidden in global space).
The same could be done for reading, possibly creating two other classes: PacketReader and PacketWriter, but you can have it both embedded in the Packet (or class Ber).
The way you have it now, you have to create all the data at once (while handling the big/little endian) and pass to constructor:
Ber(uint8_t tag, unsigned short length=0, const uint8_t *message=nullptr)
What I am proposing is to have it this way:
auto pkt = Packet(MSG_TAG) << (byte)data1 << (word)data2 << (uint)data3;
auto pkt2 = Packet(MSG_TAG, 1+2+4) << ....the same as above - space reserved ahead
auto pkt3 = Packet::construct(MSG_TAG, (byte)data1, ....);
Much easier to work with (the last one needs variadic templates, may not be accessible in your compiler).
##Finally, my proposals for naming:
Ber -> Packet
totallength -> total_length or full_length
msglength -> length (most used I think) or payload_length / data_length or message_length
lencode -> encode_length
encodedlen -> length_size (well, this one is hard to properly name)