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

initial esp-idf 3.0 build #1388

Merged
merged 15 commits into from
May 14, 2018
Merged

initial esp-idf 3.0 build #1388

merged 15 commits into from
May 14, 2018

Conversation

wilberforce
Copy link
Member

Not yet ready to merge!

@@ -672,7 +672,8 @@ int netCreateSocket(JsNetwork *net, SocketType socketType, uint32_t host, unsign
if (sckt<0) return sckt;

#ifdef USE_TLS
assert(sckt>=0 && sckt<32);
// NEED TO FIX - as socket is assumed to be < 32
//assert(sckt>=0 && sckt<32);
Copy link
Member Author

Choose a reason for hiding this comment

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

@gfwilliams
FYI @jumjum123
The move to esp-idf has changed the number range of the file descriptions. They are not longer < 32 and I believe the assert was due to the sckt number being used in flags.

Can you think of a good way to work around this issue?

Copy link
Member

Choose a reason for hiding this comment

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

I've just removed those lines. I believe they were there for TLS when we used a bitfield to store whether the socket was using TLS or not. Since @opichals work on UDP the bitfields were no longer needed \o/

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks

@wilberforce
Copy link
Member Author

Hi @jumjum123
Still getting:

targets/esp32/main.c: At top level:
targets/esp32/main.c:20:23: fatal error: bluetooth.h: No such file or directory

and when I change to #include "libs/bluetooth/bluetooth.h"

I then get:

In file included from targets/esp32/main.c:20:0:
/home/wilberforce/EspruinoBuildTools/esp32/build/Espruino/libs/bluetooth/bluetooth.h: At top level:
/home/wilberforce/EspruinoBuildTools/esp32/build/Espruino/libs/bluetooth/bluetooth.h:62:0: warning: "CENTRAL_LINK_COUNT" redefined
 #define CENTRAL_LINK_COUNT    1
 ^
/home/wilberforce/EspruinoBuildTools/esp32/build/Espruino/libs/bluetooth/bluetooth.h:57:0: note: this is the location of the previous definition
 #define CENTRAL_LINK_COUNT              0                                           /**<number of central links used by the application. When changing this number reme
 ^
In file included from targets/esp32/main.c:22:0:
targets/esp32/BLE/esp32_gap_func.h:17:29: fatal error: esp_gap_ble_api.h: No such file or directory

@jumjum123
Copy link
Contributor

@wilberforce
Hmm, how do you compile ? This is my way, could there be a problem with USE_BLUETOOTH ?

#!/bin/bash
export ESP_IDF_PATH=/home/esp32/esp-idf
export IDF_PATH=/home/esp32/esp-idf
export ESP_APP_TEMPLATE_PATH=/home/esp32/template
export BOARD=ESP32
export RTOS=1
export USE_BLUETOOTH=1
[[ ":$PATH:" != ":/home/esp32/xtensa-esp32-elf/bin:" ]] && PATH="/home/esp32/xtensa-esp32-elf/bin:${PATH}"
cd Espruino
make clean all
make

@wilberforce
Copy link
Member Author

wilberforce commented May 8, 2018

Ahhh thanks.. it needs the flag. USE_BLUETOOTH=1 looks like there are amissing #idefs in main.c

I compile like this:

BOARD=ESP32 USE_BLUETOOTH=1 make

and when tracking down errors:

BOARD=ESP32 SINGLETHREAD=1 USE_BLUETOOTH=1 make

So you don't get the parallel make.

For me - the environment is set up with source script/provision ESP32

It compiles - then fails on link:

targets/esp32/BLE/esp32_gap_func.o:(.literal.gap_event_handler+0x1c): undefined reference to `esp_ble_gap_security_rsp'
targets/esp32/BLE/esp32_gap_func.o:(.literal.gap_init_security+0x0): undefined reference to `esp_ble_gap_set_security_param'
targets/esp32/BLE/esp32_gap_func.o: In function `gap_event_handler':
/home/wilberforce/EspruinoBuildTools/esp32/build/Espruino/targets/esp32/BLE/esp32_gap_func.c:284: undefined reference to `esp_ble_gap_security_rsp'
targets/esp32/BLE/esp32_gap_func.o: In function `gap_init_security':
/home/wilberforce/EspruinoBuildTools/esp32/build/Espruino/targets/esp32/BLE/esp32_gap_func.c:306: undefined reference to `esp_ble_gap_set_security_param'
/home/wilberforce/EspruinoBuildTools/esp32/build/Espruino/targets/esp32/BLE/esp32_gap_func.c:307: undefined reference to `esp_ble_gap_set_security_param'
/home/wilberforce/EspruinoBuildTools/esp32/build/Espruino/targets/esp32/BLE/esp32_gap_func.c:308: undefined reference to `esp_ble_gap_set_security_param'
/home/wilberforce/EspruinoBuildTools/esp32/build/Espruino/targets/esp32/BLE/esp32_gap_func.c:313: undefined reference to `esp_ble_gap_set_security_param'
/home/wilberforce/EspruinoBuildTools/esp32/build/Espruino/targets/esp32/BLE/esp32_gap_func.c:314: undefined reference to `esp_ble_gap_set_security_param'

Does this look familiar?

@wilberforce
Copy link
Member Author

wilberforce commented May 8, 2018

@jumjum123
I have got it to compile with the Bluetooth flag off. Shall I push targets/ESP32/main.c upto ESP32 branch for you to pull?

I've not solved the BLE security module issue yet, - I don't think I have it compiled in the libs. Do we need this compiled in?

The difference is here:

CONFIG_BLE_SMP_ENABLE=
+CONFIG_BLE_SMP_ENABLE=y

@jumjum123
Copy link
Contributor

My development path is based on our old mate. BTW, would like to know, how Neil feels, reading this ;-)

  • first download release 3.0
  • next download template
  • set environment variables
  • run make menuconfig in template, and set configuration
  • run make in template
  • download Espruino from ESP32 branch
  • run the shell as mentioned above

Just read your latest message, yes SMP needs to be enabled

@wilberforce
Copy link
Member Author

Turning the flag on I got a build:

-rw-rw-rw- 1 wilberforce wilberforce 843168 May 7 18:32 espruino_1v97.8_esp32.bin
-rw-rw-rw- 1 wilberforce wilberforce 1129312 May 8 21:35 espruino_esp32.bin

First is without BLE, the 2nd with!

With this size - I'm not sure if we'll need to move the js_code partition anymore?

@wilberforce
Copy link
Member Author

wilberforce commented May 8, 2018

tried to boot with old partition table:

E (31) boot: ota data partition invalid, falling back to factory
E (257) esp_image: Image length 1129312 doesn't fit in partition length 983040
E (257) boot: Factory app partition is not bootable
E (257) esp_image: image at 0x200000 has invalid magic byte
E (262) boot: OTA app partition slot 0 is not bootable
E (267) boot: No bootable app partitions in the partition table
user code done

I'll see if I can get it to fit!

@jumjum123
Copy link
Contributor

my latest partition table was this, as you can see, only minor changes to have memory for firmware
Whatever we do, I would recommend users to clear flash before starting with this version

BTW, IIRC, heap in BLE version is smaller. Or with other words, heap without BLE should give the option to increase jsVars. espruinoTask in main.c already supports this for later versions with PSRAM

pin 16 and 17 are reserved for PSRAM, which we don't support yet. Please see jshPinDefaultPullup() in jshardware.c

#Name, Type, SubType, Offset, Size, Flags
#boot, data, , 0x1000, 4K
partition, data, , 0x8000, 0x1000
nvs, data, nvs, 0x9000, 16K,
#otadata, data, ota, 0xd000, 8K,
factory, app, factory, 0x10000, 1400K,
js_code, data, , 0x200000, 64K,
#ota_0, app, ota_0, 0x200000, 960K,
storage, data, , 0x300000, 1M,

@wilberforce
Copy link
Member Author

I'm thinking of making the js_code partition bigger than 64K. Say at least 128K. Then there is plenty of space for modules.

Since the firmware overlaps into 0x100000 - the next boundry is 0x200000.

We leave 0x200000-0x300000 free an have the js_code start at say 0x200000 - js_code_size.

[ 0x10000 firmware 1400K?]
[ Gap for bigger firmware or space to make js_code larger ]
[ jscode]
[0x200000-0x300000 free]

@wilberforce
Copy link
Member Author

@jumjum123

Here is the partition table - it has room for large firmware x2 ( incase we do want to do ota at some stage) and 256K for js_code!


#Name | Type | SubType | Offset | Size
-- | -- | -- | -- | --
#boot | data | 0 | 0x1000 | 4K
#reserved | 0 | 0 | 0x2000 | 24K
partition | data | 0 | 0x8000 | 4K
nvs | data | nvs | 0x9000 | 12K
otadata | data | ota | 0xC000 | 8K
free | data | 0x40 | 0xE000 | 8K
factory | app | factory | 0x10000 | 1344K
ota_0 | app | ota_0 | 0x160000 | 1344K
flash | data | 0x40 | 0x2B0000 | 64K
js_code | data | 0 | 0x2C0000 | 256K
storage | data | 0 | 0x300000 | 1024K


@wilberforce
Copy link
Member Author

wilberforce commented May 11, 2018

Issues with current build - save() not working - need to updated saved code area 0x2C0000

@wilberforce wilberforce changed the title intial esp-idf 3.0 build initial esp-idf 3.0 build May 12, 2018
@wilberforce
Copy link
Member Author

wilberforce commented May 13, 2018

@gfwilliams - Ready to merge with master please.

@jumjum123 I have pulled all the changes from the esp32 branch - except relating to the psram.
Also I have not pulled the new targets/esp32/docs - as some this will be out of date now

After this is merged, we can look at pulling the changes above into the ESP32 branch, as there have been changes to master that are not in the esp32 branch.

JSVARs are down to 2500 from 5000 ;-(.

We probably need a way to look at turning the BLE off/on and getting for jsvars avaiable when off.
I have noticed that the module runs hotter with BLE and wifi on.

if ("ifdef" in jsondata) and not (jsondata["ifdef"] in defines):
print(dropped_prefix+" because of #ifdef "+jsondata["ifdef"])
drop = True
if ("ifdef" in jsondata):
Copy link
Member

Choose a reason for hiding this comment

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

Please could we ditch this change? I assume it's because of the use of "ifdef" : "NRF52,ESP32", but everywhere else in Espruino we just use "#if" : "defined(NRF52) || defined(ESP32)" when we want to do something like this, so we should do that here (especially as it's not mirrored for ifndef, and the documentation won't be created correctly for it either).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I will get to this tonight - it's 8pm here. I'll try

Copy link
Contributor

@jumjum123 jumjum123 May 14, 2018

Choose a reason for hiding this comment

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

Got your OK some time ago, to do it this way ;-)
I didn't know better, therefor I added my way.
At least the documentation is a big point.
@wilberforce, where should I do these changes. I would recommend to use ESP32-v3.0 branch

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes please - ESP32-v3.0 branch. I have spent many hours merging carefully so nothing breaks ;-)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Sorry about the confusion @jumjum123 - I guess I wasn't paying attention at the time :(

@@ -33,6 +33,7 @@ typedef enum {
#define BLETASK_IS_CENTRAL(x) ((x)>=BLETASK_CENTRAL_START && ((x)<=BLETASK_CENTRAL_END))

extern JsVar *bleTaskInfo; // info related to the current task
extern JsVar *blePromise; //defined here, used in jswrap_bluetooth.c and in ESP32 relevant bluetooth
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this? I'm not sure it is used and it'd be good to keep it private - ble*Task functions should hopefully provide everything that's needed to deal with the promises.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not too familar with the code here @jumjum123 did this.... Do you mean put in in a #ifdef ESP32 ? Or ditch it all together?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have in mind, I had to do this for some reason.
But don't remember why :-(
Something like "ran into error during compiling"

Copy link
Member Author

Choose a reason for hiding this comment

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

@jumjum123
Looks like this can be removed as it's not getting used at this point.

@gfwilliams
Copy link
Member

gfwilliams commented May 14, 2018

Looks great! is it possible to tweak the above two things? Otherwise I can do it when merging - it won't take long.

@jumjum123
Copy link
Contributor

First of all big THANKS to @wilberforce and @gfwilliams
It's a big moment to see my BLE changes making it to master.
@wilberforce, I would use the branches ESP32 and ESP32-v3.0 in the way to

  • keep ESP32-v3.0 as the master for ESP32
  • use ESP32 as a kind of playground for anything, that relies on Espressif Master, like PSRAM support
    Would you agree ?
    @wilberforce, I've seen you using #include "libs/bluetooth/bluetooth.h".
    In my understanding #include bluetooth.h" should be ok.
    In Makefile line 546, we have this INCLUDE += -I$(ROOT)/libs/bluetooth
    Why do we need the long include ?

@gfwilliams if you could do the changes, would be a big help. You are much faster than I could ever be.

@wilberforce, as soons as I get UART running, my next step will be to check how we can switch off BLE during runtime. I've some ideas for testing. Hopefully, we will get some memory back for jsVars
I'm still disappointed by Espressif, removing correct support of PSRAM from release.

@gfwilliams
Copy link
Member

"libs/bluetooth/bluetooth.h".

Yeah, that could just be bluetooth.h. Style-wise, using full path might have been better but we're a long way down this path now and it's probably better to stick with the same thing we do elsewhere.

Sure, I'll tweak and merge (since I merged some other PRs there's a minor conflict I need to tweak anyway). Probably won't be until much later today though as I've got a bunch of Pixl boards to pack and send out.

@wilberforce
Copy link
Member Author

wilberforce commented May 14, 2018

@gfwilliams

I've cleaned up the promise var, and the conditional builds - and tried to revert common.py

I'm getting a link error now - possibly broke the wrapper code????

gen/jswrapper.o:(.rodata.jswSymbols_E+0x34): undefined reference to `jswrap_espruino_dumpFreeList'
gen/jswrapper.o:(.rodata.jswSymbols_E+0x3c): undefined reference to `jswrap_espruino_dumpLockedVars'

Let's see what the travis build comes back with.

I've broken something. Bugger.

I'm out of time, so will sort tomorrow unless you get to it first.

@wilberforce
Copy link
Member Author

@gfwilliams

and I have added to the change log incorrectly ;-() put the thing at the top rather than the bottom:

ESP32: update esp-idf to v3.0. BLE support - thanks to @jumjum. Erase flash before flashing. vars now 2500

@wilberforce
Copy link
Member Author

@gfwilliams - looks like all good now - just the Changelog to sort!

@gfwilliams
Copy link
Member

Awesome - thanks for doing that!

@gfwilliams gfwilliams merged commit c9ae999 into master May 14, 2018
@gfwilliams
Copy link
Member

All merged - thanks for all your work on this everyone!

@jumjum123 I will get a global BLE == NRF var added for this soon - promise :)

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.

None yet

3 participants