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

Upgrade esp8266 to SDK v2.0.0.patch1 #925

Merged
merged 24 commits into from Sep 20, 2016
Merged

Upgrade esp8266 to SDK v2.0.0.patch1 #925

merged 24 commits into from Sep 20, 2016

Conversation

@tve
Copy link
Contributor

@tve tve commented Sep 17, 2016

Recent pull requests have enabled crypto and graphics, which has put the size of the binary above the actual space available. I've disabled graphics to make things fit. If someone wants to propose a layout change or work on reducing the size that would be awesome. But just changing the max size before the Makefile bails out is not a solution...

@nkolban nkolban mentioned this pull request Sep 17, 2016
@wilberforce
Copy link
Member

@wilberforce wilberforce commented Sep 18, 2016

@tve
Your SDK 2.0 updates superseed the updates I had done...

https://github.com/espruino/Espruino/pull/924/files

I'm not sure if your's also updated the docker file and the flash Readme?

@tve
Copy link
Contributor Author

@tve tve commented Sep 18, 2016

Mhh, I wasn't aware of the Dockerfile, I'll fix it, thanks for pointing it out.

@wilberforce
Copy link
Member

@wilberforce wilberforce commented Sep 19, 2016

From the forum:

Wilberforce: Is version 2.0 sdk larger - is that why the rom size had to drop, and therefore the graphics library?
tve: No, it had to drop because it never fit. It was in the previous build because someone just changed the max size in the makefile without considering whether there actually is space. So code overlapped the eeprom flash area and other stuff.

The Makefile update for the available rom was here:

60a1698#diff-b67911656ef5d18c4ae36cb6741b7965R509

New change:
-ESP_FLASH_MAX ?= 491520 # max bin file: 480KB

+ESP_FLASH_MAX ?= 479232 # max bin file: 468KB

So 480 reserves 32K and 468 reserves 44K.

I'm not sure I understand how you got this value?

@tve
Copy link
Contributor Author

@tve tve commented Sep 19, 2016

The flash layout is documented in https://github.com/espruino/EspruinoDocs/blob/master/boards/EspruinoESP8266.md#flash-map-and-access
The user1.bin image starts at 0x1000 and the SDK RF calibration save area on 512KB modules starts at 0x076000. So 0x75000=479232 is the space available.

@wilberforce
Copy link
Member

@wilberforce wilberforce commented Sep 19, 2016

Ahhh - so this is for the 512M modules.

The larger ones are not affected - They could still be built with Crypto and graphics?

My concern is that people will be surprised if they were using the graphics module and it disappear from the firmware.

Are we asking for trouble if then Makefile switched in graphics depending on the rom size?

That's not going to work with the universal build is it?

@tve
Copy link
Contributor Author

@tve tve commented Sep 19, 2016

There is only a single build, so there is no notion of switching with ROM size. If someone wants to go in and move things around, that's fine with me. I don't use 512KB modules. Could also eliminate eeprom area and one of the flash save blocks for 512kb modules. But making those changes requires code changes and conditionals in a number of places. I just wanted to update to the latest SDK and add some fixes I've had queued up and I don't really have the energy to change flash layout at this point.

@tve
Copy link
Contributor Author

@tve tve commented Sep 19, 2016

I just ran the build again with USE_GRAPHICS, the problem is:
** user1.bin uses 488788 bytes of 479232 available
so this is 9556 bytes over, meaning 3 flash sectors (4KB each) would have to be freed up for it to fit.
I don't know whether it's possible to cut down the amount of space used by the graphics stuff.

@gfwilliams
Copy link
Member

@gfwilliams gfwilliams commented Sep 19, 2016

Thanks - I notice you've added -g to OPTIMIZEFLAGS - could that be causing the code size increase?

Also, just IMO but wouldn't just having Wifi.setTelnetOptions or something be nicer than having both TelnetServer and Telnet libraries?

@tve
Copy link
Contributor Author

@tve tve commented Sep 19, 2016

WRT -g I did a build without -g and the difference is less than 100 bytes, IIRC with -g was smaller, actually.

WRT Telnet I need to go back. The problem I was having was that there is both a class called Telnet and a device called Telnet. I renamed the class to TelnetServer so it's not shadowed by the device. If this doesn't make sense, I can go back and see what I was smoking...

@gfwilliams
Copy link
Member

@gfwilliams gfwilliams commented Sep 20, 2016

Thanks for the explanation. Odd about -g (although I never tried it with -Os as well).

Well, it's ok - it'd be a lot tidier if we could do it with just the Telnet class, but I remember you had issues. What about Telnet.setup? presumably that would be available as part of Serial - in the standard handler you could detect if you were dealing with Telnet then forward the setup call elsewhere?

@gfwilliams gfwilliams merged commit 8f25289 into espruino:master Sep 20, 2016
1 check failed
@tve
Copy link
Contributor Author

@tve tve commented Sep 20, 2016

Sorry, I'm not understanding your comment about Telnet.setup

@gfwilliams
Copy link
Member

@gfwilliams gfwilliams commented Sep 20, 2016

Well, you would have done TelnetServer.setOptions(options) but you could just do Telnet.setup(options).

Because Telnet is a serial device, it should have a setup method already which will probably just produce an error. You could just use that method to set the options needed for Telnet, rather than needing a new class?

@tve
Copy link
Contributor Author

@tve tve commented Sep 20, 2016

Mhh, I'm gonna have to revert the change and re-test. What I thought was happening is that I would use Telnet.setConsole() but instead of calling setConsole on the Telnet device that code was attempting to call setConsole on the Telnet class, which doesn't have that as a class method.

@gfwilliams
Copy link
Member

@gfwilliams gfwilliams commented Sep 20, 2016

Yeah, I think it's getting confused between the named variable and the class name because that whole system is a bit iffy (there is a branch to fix it, but it's a way off being ready). So are you saying that with your change, Telnet.setOptions works? If so, great - It hadn't occurred to me that's what you might be doing,

I think by using .setup you can avoid defining a Telnet class (and just use the device), and then everything would work as expected? Since Telnet works on Linux this is one of those things that will probably be easier to test on there :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants