Skip to content
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.

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

wifinina: add generated strings, improved debugging system and messages #560

Merged
merged 2 commits into from
May 14, 2023

Conversation

deadprogram
Copy link
Member

This PR adds generated strings, improved debugging system and improved error messages to the wifinina driver.

@deadprogram
Copy link
Member Author

With this PR:

sendCmd: Start StartClientTCP(2D) 04 (3)
sendParam8: 0 lastParam: false 
sendParam8: 0 lastParam: true 
spiChipDeselect
waitForChipReady()
spiChipDeselect
wifinina error: TimeoutChipReady

@deadprogram
Copy link
Member Author

The only drawback of this PR is that it does increase the resulting size by nearly 1.5k:

Before this PR:

tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/wifinina/ntpclient/main.go
go: downloading golang.org/x/net v0.0.0-20210614182718-04defd469f4e
go: downloading golang.org/x/text v0.3.6
   code    data     bss |   flash     ram
 134492    2468    6056 |  136960    8524
5a9025262f3decd7b331a8b8f0c54d75  ./build/test.hex
tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/wifinina/udpstation/main.go
   code    data     bss |   flash     ram
 134492    2464    6064 |  136956    8528
adc31f618547adcbfe5d2eeb2cb274bd  ./build/test.hex
tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/wifinina/tcpclient/main.go
   code    data     bss |   flash     ram
 134700    2476    6056 |  137176    8532
526bb08ba389a3b6b12d9ffa88e93e50  ./build/test.hex
tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/wifinina/webclient/main.go
   code    data     bss |   flash     ram
 135024    2476    6056 |  137500    8532
3c00af7c526b1f4d52566a56cf797eee  ./build/test.hex

With this PR

tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/wifinina/ntpclient/main.go
go: downloading golang.org/x/net v0.0.0-20210614182718-04defd469f4e
go: downloading golang.org/x/text v0.3.6
   code    data     bss |   flash     ram
 136096    2468    6056 |  138564    8524
cd7b727cf628b948394e9d8050c7aa0d  ./build/test.hex
tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/wifinina/udpstation/main.go
   code    data     bss |   flash     ram
 136096    2464    6064 |  138560    8528
374065f1ff7d224cb2ccdb5119cf1891  ./build/test.hex
tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/wifinina/tcpclient/main.go
   code    data     bss |   flash     ram
 136[308](https://github.com/tinygo-org/drivers/actions/runs/4841230648/jobs/8627335753#step:8:309)    2476    6056 |  138784    8532
2ec94c900d92e688cdd9dbf6176ca245  ./build/test.hex
tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/wifinina/webclient/main.go
   code    data     bss |   flash     ram
 136628    2476    6056 |  139104    8532
b7c636add26b4ade799ce248c3e50bb7  ./build/test.hex

@aykevl
Copy link
Member

aykevl commented May 1, 2023

Oof, that's a big increase!
Not sure about this. (I generally have less of a problem with binary size in drivers because they're easier to avoid / optimize, but this is still quite a lot!).
Here is one alternative idea: log the error code and a link (possibly a shortlink) to a page where the error message can be looked up easily. That shouldn't result in a big code size increase, although it is still a bit less usable.

(I won't block this PR over the code size increase if you think it's worth it, just thinking of alternatives that might reduce the binary size somewhat).

@deadprogram
Copy link
Member Author

deadprogram commented May 1, 2023

OK, I just changed this to put most of the strings behind the wifidebug tag and still leave better error strings at cost of only 450 bytes.

Size before this PR:

$ tinygo build -size short -o test.hex -target pyportal -ldflags="-X main.ssid=rems -X main.pass=Salvador1" ./examples/wifinina/mqttsub/
   code    data     bss |   flash     ram
 180120    3036    8120 |  183156   11156

This PR without the wifidebug tag:

$ tinygo build -size short -o test.hex -target pyportal -ldflags="-X main.ssid=XXX -X main.pass=YYY" ./examples/wifinina/mqttsub/          
   code    data     bss |   flash     ram                                                                                                         
 180576    3036    8120 |  183612   11156

This PR with the wifidebug tag:

$ tinygo build -size short -o test.hex -target pyportal -ldflags="-X main.ssid=XXX -X main.pass=YYY" -tags=wifidebug ./examples/wifinina/mq
ttsub/                                                                                                                                            
   code    data     bss |   flash     ram                                                                                                         
 183048    3036    8120 |  186084   11156

@deadprogram
Copy link
Member Author

Now that I have been testing #537 I am going to close this PR. The error strings can be added in a separate PR once #537 makes it in.

@deadprogram deadprogram closed this May 4, 2023
@deadprogram deadprogram reopened this May 9, 2023
@deadprogram
Copy link
Member Author

Reopening since #537 will not merge until TinyGo 0.29

@deadprogram
Copy link
Member Author

Since this is now only 450 bytes size increase in normal (non-debug) mode, I am going to merge. We really do need it.

@deadprogram deadprogram merged commit 9955e09 into dev May 14, 2023
@deadprogram deadprogram deleted the wifinina-generate-strings branch May 14, 2023 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants