Skip to content

Rewrite IPAddress without union #8819

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

Closed
wants to merge 4 commits into from
Closed

Rewrite IPAddress without union #8819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cba06d1
Fix ambiguous for TwoWire::requestFrom() methods.
safocl Oct 30, 2023
14660ef
Remove TwoWire::begin(int) overload
safocl Oct 30, 2023
354608f
Rewrite IPAddress without union
safocl Oct 31, 2023
5d65a54
Fix align for IPAddress.
safocl Nov 3, 2023
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
46 changes: 23 additions & 23 deletions cores/esp32/IPAddress.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,62 +20,62 @@
#include <Arduino.h>
#include <IPAddress.h>
#include <Print.h>
#include <algorithm>

IPAddress::IPAddress()
{
_address.dword = 0;
}
// IPAddress::IPAddress()
// {
// _address.dword = 0;
// }

IPAddress::IPAddress(uint8_t first_octet, uint8_t second_octet, uint8_t third_octet, uint8_t fourth_octet)
{
_address.bytes[0] = first_octet;
_address.bytes[1] = second_octet;
_address.bytes[2] = third_octet;
_address.bytes[3] = fourth_octet;
}
: _address({first_octet,
second_octet,
third_octet,
fourth_octet})
{}

IPAddress::IPAddress(uint32_t address)
{
_address.dword = address;
uint32_t& addressRef = reinterpret_cast<uint32_t&>(_address.front());
addressRef = address;
}

IPAddress::IPAddress(const uint8_t *address)
{
memcpy(_address.bytes, address, sizeof(_address.bytes));
}
IPAddress::IPAddress(const uint8_t *address) : _address({address[0], address[1], address[2], address[3]})
{}

IPAddress& IPAddress::operator=(const uint8_t *address)
{
memcpy(_address.bytes, address, sizeof(_address.bytes));
std::copy(address, address + _address.size(), _address.begin());
return *this;
}

IPAddress& IPAddress::operator=(uint32_t address)
{
_address.dword = address;
uint32_t& addressRef = reinterpret_cast<uint32_t&>(_address.front());
Copy link
Contributor

Choose a reason for hiding this comment

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

is _address.front() guaranteed to be 32-bit aligned?
If not, then this will cause undefined behavior.

Copy link
Contributor Author

@safocl safocl Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

is _address.front() guaranteed to be 32-bit aligned? If not, then this will cause undefined behavior.

It is guaranteed be aligned as structure -- array is once member in this structure -- it has same address. It behavior same as union or c-style array (uint8_t a[4]).

Standart says (https://timsong-cpp.github.io/cppwp/n4659/array):

class template for storing fixed-size sequences of objects. An array is a contiguous container. An instance of array<T, N> stores N elements of type T, so that size() == N is an invariant.

An array is an aggregate that can be list-initialized with up to N elements whose types are convertible to T.

https://timsong-cpp.github.io/cppwp/n4659/dcl.init.aggr#1:

An aggregate is an array or a class with

(1.1) no user-provided, explicit, or inherited constructors ([class.ctor]),
(1.2) no private or protected non-static data members (Clause [class.access]),
(1.3) no virtual functions, and
(1.4) no virtual, private, or protected base classes ([class.mi]).
[ Note: Aggregate initialization does not allow accessing protected and private base class' members or constructors.  — end note ]

https://timsong-cpp.github.io/cppwp/n4659/dcl.init.aggr#2:

2 The elements of an aggregate are:

(2.1) for an array, the array elements in increasing subscript order, or
(2.2) for a class, the direct base classes in declaration order, followed by the direct non-static data members ([class.mem]) that are not members of an anonymous union, in declaration order.

and (https://timsong-cpp.github.io/cppwp/n4659/container.requirements#general-6):

begin() returns an iterator referring to the first element in the container. end() returns an iterator which is the past-the-end value for the container. If the container is empty, then begin() == end().

https://timsong-cpp.github.io/cppwp/n4659/sequence.reqmts#tab:containers.sequence.optional:

a.front() = *a.begin()

It mean that &_address.front() == &(*_address.begin()) == &_address == &IPAddress
and correspondingly an align of _address.front() depend on an align of IPAddress structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but this example shows that alignof = 1 for struct with std::array<uint8_t,4> member.
alignas(4) is required to achieve 32-bit alignment

struct alignas(4) Sarray { 
std::array<uint8_t,4> a{};
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but this example shows that class IPAddress has 4 bytes of aligment because it inherits from an abstract type (with vtable).

in this case, the _address field still has 1 byte of alignment. -- need alignas( alignof(uint32_t) ) for IPAddress.

struct IPAddress: public Printable
{
    alignas( alignof(uint32_t) ) std::array<uint8_t,4> _address{};
};

as here

I didn’t initially take into account that inheritance occurs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i fix this omission.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here, of course, I would like to generally highlight that uint32_t may not exist at all on a specific platform.
Maybe uint_least32_t should be used for compatibility?

Copy link
Contributor

Choose a reason for hiding this comment

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

uint32_t is known in all platforms this repository covers.
I don't know uint_least32_t, but it seems to be not guaranteed to be a fixed length type, which will cause issues when it is used in some struct of fixed length or offset of members (e.g. storing a struct or transferring it to another node)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know uint_least32_t, but it seems to be not guaranteed to be a fixed length type, which will cause issues when it is used in some struct of fixed length or offset of members (e.g. storing a struct or transferring it to another node)

https://timsong-cpp.github.io/cppwp/n4659/cstdint#syn -- types that are at least N-bit

addressRef = address;
return *this;
}

bool IPAddress::operator==(const uint8_t* addr) const
{
return memcmp(addr, _address.bytes, sizeof(_address.bytes)) == 0;
return std::equal(_address.begin(), _address.end(), addr);
}

size_t IPAddress::printTo(Print& p) const
{
size_t n = 0;
for(int i = 0; i < 3; i++) {
n += p.print(_address.bytes[i], DEC);
n += p.print(_address[i], DEC);
n += p.print('.');
}
n += p.print(_address.bytes[3], DEC);
n += p.print(_address[3], DEC);
return n;
}

String IPAddress::toString() const
{
char szRet[16];
sprintf(szRet,"%u.%u.%u.%u", _address.bytes[0], _address.bytes[1], _address.bytes[2], _address.bytes[3]);
sprintf(szRet,"%u.%u.%u.%u", _address[0], _address[1], _address[2], _address[3]);
return String(szRet);
}

Expand Down Expand Up @@ -103,7 +103,7 @@ bool IPAddress::fromString(const char *address)
// Too much dots (there must be 3 dots)
return false;
}
_address.bytes[dots++] = acc;
_address[dots++] = acc;
acc = 0;
}
else
Expand All @@ -117,7 +117,7 @@ bool IPAddress::fromString(const char *address)
// Too few dots (there must be 3 dots)
return false;
}
_address.bytes[3] = acc;
_address[3] = acc;
return true;
}

Expand Down
19 changes: 9 additions & 10 deletions cores/esp32/IPAddress.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,29 +23,27 @@
#include <stdint.h>
#include <WString.h>
#include <Printable.h>
#include <array>

// A class to make it easier to handle and pass around IP addresses

class IPAddress: public Printable
{
private:
union {
uint8_t bytes[4]; // IPv4 address
uint32_t dword;
} _address;
alignas(alignof(uint32_t)) std::array<uint8_t,4> _address{}; // IPv4 address

// Access the raw byte array containing the address. Because this returns a pointer
// to the internal structure rather than a copy of the address this function should only
// be used when you know that the usage of the returned uint8_t* will be transient and not
// stored.
uint8_t* raw_address()
{
return _address.bytes;
return _address.data();
}

public:
// Constructors
IPAddress();
IPAddress() = default;
IPAddress(uint8_t first_octet, uint8_t second_octet, uint8_t third_octet, uint8_t fourth_octet);
IPAddress(uint32_t address);
IPAddress(const uint8_t *address);
Expand All @@ -54,26 +52,27 @@ class IPAddress: public Printable
bool fromString(const char *address);
bool fromString(const String &address) { return fromString(address.c_str()); }


// Overloaded cast operator to allow IPAddress objects to be used where a pointer
// to a four-byte uint8_t array is expected
operator uint32_t() const
{
return _address.dword;
return reinterpret_cast<const uint32_t&>(_address.front());
Copy link
Contributor

Choose a reason for hiding this comment

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

Idem, reinterpret_cast should be used with utmost care, especially with alignment.

}
bool operator==(const IPAddress& addr) const
{
return _address.dword == addr._address.dword;
return _address == addr._address;
}
bool operator==(const uint8_t* addr) const;

// Overloaded index operator to allow getting and setting individual octets of the address
uint8_t operator[](int index) const
{
return _address.bytes[index];
return _address[index];
}
uint8_t& operator[](int index)
{
return _address.bytes[index];
return _address[index];
}

// Overloaded copy operators to allow initialisation of IPAddress objects from other types
Expand Down
84 changes: 42 additions & 42 deletions libraries/Wire/src/Wire.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,48 +588,48 @@ void TwoWire::flush(void)
//i2cFlush(num); // cleanup
}

size_t TwoWire::requestFrom(uint8_t address, size_t len, bool sendStop)
{
return requestFrom(static_cast<uint16_t>(address), static_cast<size_t>(len), static_cast<bool>(sendStop));
}

uint8_t TwoWire::requestFrom(uint8_t address, uint8_t len, uint8_t sendStop)
{
return requestFrom(static_cast<uint16_t>(address), static_cast<size_t>(len), static_cast<bool>(sendStop));
}

uint8_t TwoWire::requestFrom(uint16_t address, uint8_t len, uint8_t sendStop)
{
return requestFrom(address, static_cast<size_t>(len), static_cast<bool>(sendStop));
}

/* Added to match the Arduino function definition: https://github.com/arduino/ArduinoCore-API/blob/173e8eadced2ad32eeb93bcbd5c49f8d6a055ea6/api/HardwareI2C.h#L39
* See: https://github.com/arduino-libraries/ArduinoECCX08/issues/25
*/
uint8_t TwoWire::requestFrom(uint16_t address, uint8_t len, bool stopBit)
{
return requestFrom((uint16_t)address, (size_t)len, stopBit);
}

uint8_t TwoWire::requestFrom(uint8_t address, uint8_t len)
{
return requestFrom(static_cast<uint16_t>(address), static_cast<size_t>(len), true);
}

uint8_t TwoWire::requestFrom(uint16_t address, uint8_t len)
{
return requestFrom(address, static_cast<size_t>(len), true);
}

uint8_t TwoWire::requestFrom(int address, int len)
{
return requestFrom(static_cast<uint16_t>(address), static_cast<size_t>(len), true);
}

uint8_t TwoWire::requestFrom(int address, int len, int sendStop)
{
return static_cast<uint8_t>(requestFrom(static_cast<uint16_t>(address), static_cast<size_t>(len), static_cast<bool>(sendStop)));
}
// size_t TwoWire::requestFrom(uint8_t address, size_t len, bool sendStop)
// {
// return requestFrom(static_cast<uint16_t>(address), static_cast<size_t>(len), static_cast<bool>(sendStop));
// }
//
// uint8_t TwoWire::requestFrom(uint8_t address, uint8_t len, uint8_t sendStop)
// {
// return requestFrom(static_cast<uint16_t>(address), static_cast<size_t>(len), static_cast<bool>(sendStop));
// }
//
// uint8_t TwoWire::requestFrom(uint16_t address, uint8_t len, uint8_t sendStop)
// {
// return requestFrom(address, static_cast<size_t>(len), static_cast<bool>(sendStop));
// }
//
// /* Added to match the Arduino function definition: https://github.com/arduino/ArduinoCore-API/blob/173e8eadced2ad32eeb93bcbd5c49f8d6a055ea6/api/HardwareI2C.h#L39
// * See: https://github.com/arduino-libraries/ArduinoECCX08/issues/25
// */
// uint8_t TwoWire::requestFrom(uint16_t address, uint8_t len, bool stopBit)
// {
// return requestFrom((uint16_t)address, (size_t)len, stopBit);
// }
//
// uint8_t TwoWire::requestFrom(uint8_t address, uint8_t len)
// {
// return requestFrom(static_cast<uint16_t>(address), static_cast<size_t>(len), true);
// }
//
// uint8_t TwoWire::requestFrom(uint16_t address, uint8_t len)
// {
// return requestFrom(address, static_cast<size_t>(len), true);
// }
//
// uint8_t TwoWire::requestFrom(int address, int len)
// {
// return requestFrom(static_cast<uint16_t>(address), static_cast<size_t>(len), true);
// }
//
// uint8_t TwoWire::requestFrom(int address, int len, int sendStop)
// {
// return static_cast<uint8_t>(requestFrom(static_cast<uint16_t>(address), static_cast<size_t>(len), static_cast<bool>(sendStop)));
// }

void TwoWire::beginTransmission(int address)
{
Expand Down
24 changes: 10 additions & 14 deletions libraries/Wire/src/Wire.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,7 @@ class TwoWire: public Stream
{
return begin(addr, -1, -1, 0);
}
inline bool begin(int addr)
{
return begin(static_cast<uint8_t>(addr), -1, -1, 0);
}
bool end();
bool end();

size_t setBufferSize(size_t bSize);

Expand All @@ -121,15 +117,15 @@ class TwoWire: public Stream
uint8_t endTransmission(bool sendStop);
uint8_t endTransmission(void);

size_t requestFrom(uint16_t address, size_t size, bool sendStop);
uint8_t requestFrom(uint16_t address, uint8_t size, bool sendStop);
uint8_t requestFrom(uint16_t address, uint8_t size, uint8_t sendStop);
size_t requestFrom(uint8_t address, size_t len, bool stopBit);
uint8_t requestFrom(uint16_t address, uint8_t size);
uint8_t requestFrom(uint8_t address, uint8_t size, uint8_t sendStop);
uint8_t requestFrom(uint8_t address, uint8_t size);
uint8_t requestFrom(int address, int size, int sendStop);
uint8_t requestFrom(int address, int size);
size_t requestFrom(uint16_t address, size_t size, bool sendStop = true);
// uint8_t requestFrom(uint16_t address, uint8_t size, bool sendStop);
// uint8_t requestFrom(uint16_t address, uint8_t size, uint8_t sendStop);
// size_t requestFrom(uint8_t address, size_t len, bool stopBit);
// uint8_t requestFrom(uint16_t address, uint8_t size);
// uint8_t requestFrom(uint8_t address, uint8_t size, uint8_t sendStop);
// uint8_t requestFrom(uint8_t address, uint8_t size);
// uint8_t requestFrom(int address, int size, int sendStop);
// uint8_t requestFrom(int address, int size);

size_t write(uint8_t);
size_t write(const uint8_t *, size_t);
Expand Down