search

tinygo-org / drivers

TinyGo drivers for sensors, displays, wireless adaptors, and other devices that use I2C, SPI, GPIO, ADC, and UART interfaces.
https://tinygo.org
BSD 3-Clause "New" or "Revised" License
587 stars 182 forks source link

(#501) make IP.String() method return something sensible #502

Closed vjanelle closed 1 year ago

deadprogram commented 1 year ago

Hello @vjanelle thanks for pointing out this problem.

It would be better to solve it in a way that avoid using the fmt package, because that package makes heavy use of reflection which greatly increases the memory footprint.

Also, please make all PRs against the dev branch. Please see https://github.com/tinygo-org/drivers/blob/release/CONTRIBUTING.md#how-to-use-our-github-repository for more info.

Thank you!

vjanelle commented 1 year ago

np! I saw somewhere else this being done with just concat, I'll try that instead.

vjanelle commented 1 year ago

@deadprogram reimplemented, added a test to cover the change

deadprogram commented 1 year ago

Thank you very much @vjanelle now merging.

deadprogram commented 1 year ago

I did not realize it at the time I reviewed this PR, but it has the unintentional side effect of breaking code such as the Drivers net/mqtt packages. The mismatch between the IP wifinina package and the net package result in invalid IP addresses being used during actual usage.

Since the upcoming refactor of netdev changes this for the actual correct Go implementation, I think it is better to just revert this PR instead of trying to fix it.

Sorry I did not do a more complete review/testing at the time @vjanelle and hope it does not inconvenience.