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 commit for embedding JerryScript to specific target boards. #733

Merged
merged 4 commits into from Jan 13, 2016

Conversation

Projects
None yet
4 participants
@seanshpark
Contributor

seanshpark commented Nov 24, 2015

This change includes mbed/frdm-k64f and esp8266-01.

JerryScript-DCO-1.0-Signed-off-by: SaeHie Park saehie.park@samsung.com

@egavrin

This comment has been minimized.

Contributor

egavrin commented Nov 26, 2015

Please split your commit into 3 commits:

  • Infrastructural changes
  • +ESP
  • +K65F
@egavrin

This comment has been minimized.

Contributor

egavrin commented Nov 26, 2015

Please rename embedding into targets

@@ -0,0 +1,4 @@
user/esp8266_js.h

This comment has been minimized.

@egavrin

egavrin Nov 26, 2015

Contributor

Do we need this file here?

This comment has been minimized.

@seanshpark

seanshpark Nov 26, 2015

Contributor

this file is generated from js soure files to embed into memory.

This comment has been minimized.

@egavrin

egavrin Dec 1, 2015

Contributor

I mean .gitignore?

This comment has been minimized.

@seanshpark

seanshpark Dec 1, 2015

Contributor

yes, to ignore build time generated files and it be dependent on targets. can I ask if you have some other things in mind?

This comment has been minimized.

@egavrin

egavrin Dec 2, 2015

Contributor

I mean .gitignore file itself. We can update the root .gitignore?

This comment has been minimized.

@seanshpark

seanshpark Dec 7, 2015

Contributor

yes, merge to root .gitignore is ok.

@@ -0,0 +1,5 @@
libjerry/

This comment has been minimized.

@egavrin

egavrin Nov 26, 2015

Contributor

Do we need this file here?

This comment has been minimized.

@seanshpark

seanshpark Nov 26, 2015

Contributor

this folder is where jerry libraries are copied, so I think it won't be necessary to include to repo

@egavrin

This comment has been minimized.

Contributor

egavrin commented Nov 26, 2015

Please share guide on how to build engine/firmware for K64F.

@egavrin

This comment has been minimized.

Contributor

egavrin commented Nov 26, 2015

Do you plan to move STM32* build to this infra?

#### 1. SDK
Follow [this](https://github.com/seanshpark/esp8266-docs/wiki/Preparation-and-setting-build-environment)

This comment has been minimized.

@egavrin

egavrin Nov 26, 2015

Contributor

Great guide! What do you think about moving this guide into source dir of the project? i.e ./docs/ESP-PREREQUISITES.md

This comment has been minimized.

@seanshpark

seanshpark Nov 27, 2015

Contributor

Thank you :). Yes, I need this kind of comments. I'll put a copy.

/** Characters */
lit_utf8_byte_t data[ sizeof (uint64_t) - sizeof (mem_cpointer_t) ];
/** Compressed pointer to next chunk */
mem_cpointer_t next_chunk_cp;

This comment has been minimized.

@egavrin

egavrin Nov 26, 2015

Contributor

Please move this fix into a separate commit.

This comment has been minimized.

@seanshpark

seanshpark Nov 27, 2015

Contributor

OK

@@ -120,7 +120,11 @@ extern void __noreturn jerry_unimplemented (const char *, const char *, const ch
#define JERRY_DDDLOG(...) JERRY_DLOG (__VA_ARGS__)
#endif /* !JERRY_ENABLE_LOG */
#if defined (__TARGET_ESP8266)
#define JERRY_ERROR_MSG(...) printf (__VA_ARGS__)

This comment has been minimized.

@egavrin

egavrin Nov 26, 2015

Contributor

I think we should keep core sources as much target independent as possible, please implement fprintf in ESP8266 sources.

This comment has been minimized.

@seanshpark

seanshpark Nov 27, 2015

Contributor

Yes, I totally agree. fprintf exist in ESP8266 SDK libcirom.a, but I have undefined reference to '_impure_ptr' link error. I need some help one this.

@@ -1005,11 +1005,19 @@ lit_put_ecma_char (ecma_char_t ecma_char) /**< code unit */
{
if (ecma_char <= LIT_UTF8_1_BYTE_CODE_POINT_MAX)
{
putchar (ecma_char);
#if defined (__TARGET_ESP8266)

This comment has been minimized.

@egavrin

egavrin Nov 26, 2015

Contributor

Likewise.

This comment has been minimized.

@egavrin

egavrin Nov 26, 2015

Contributor

Actually, this printf shouldn't be here - it looks like issue. We should use LOG_LEVEL for that.
Please open issue about this.

}
else
{
FIXME ("Support unicode characters printing.");
#if defined (__TARGET_ESP8266)
printf ("_");

This comment has been minimized.

@egavrin

egavrin Nov 26, 2015

Contributor

Likewise.

#if defined (__TARGET_ESP8266)
printf ("%c", ecma_char);
#else
putchar ((char) ecma_char);

This comment has been minimized.

@egavrin

egavrin Nov 26, 2015

Contributor

Likewise.

This comment has been minimized.

@seanshpark

seanshpark Nov 27, 2015

Contributor

OK, an issue we can talk about :)

@egavrin egavrin assigned seanshpark and unassigned egavrin Nov 26, 2015

@seanshpark

This comment has been minimized.

Contributor

seanshpark commented Nov 26, 2015

Please share guide on how to build engine/firmware for K64F.

readme.md file in mbedk64f has a link, http://yottadocs.mbed.com/#installing. maybe thinking of some short version?

@seanshpark

This comment has been minimized.

Contributor

seanshpark commented Nov 27, 2015

Do you plan to move STM32* build to this infra?

You mean current bare bone version? well, no. When this target embedding structure be stable and lands, I would like someone interested can do.

@seanshpark

This comment has been minimized.

Contributor

seanshpark commented Nov 27, 2015

I'll update this change when #752 is settled down.

@seanshpark

This comment has been minimized.

Contributor

seanshpark commented Nov 27, 2015

Please split your commit into 3 commits

Sure, it's important to make one thing at a time.

@@ -419,11 +425,11 @@ endif()
target_include_directories(${TARGET_NAME} SYSTEM PRIVATE ${INCLUDE_EXTERNAL_LIBS_INTERFACE})
if(("${PLATFORM}" STREQUAL "DARWIN") AND (NOT (CMAKE_COMPILER_IS_GNUCC OR CMAKE_COMPILER_IS_GNUCXX)))
target_link_libraries(${TARGET_NAME} ${CORE_TARGET_NAME} ${LIBC_TARGET_NAME}
${FDLIBM_TARGET_NAME} ${PREFIX_IMPORTED_LIB}libclang_rt.osx)
else()
${FDLIBM_TARGET_NAME} ${PREFIX_IMPORTED_LIB}libclang_rt.osx)

This comment has been minimized.

@egavrin

egavrin Dec 2, 2015

Contributor

Accident reformatting?

This comment has been minimized.

@seanshpark

seanshpark Dec 2, 2015

Contributor

It's removal of TAB. This part is already landed by 5ffc9ab

target_link_libraries(${TARGET_NAME} ${CORE_TARGET_NAME} ${LIBC_TARGET_NAME}
${FDLIBM_TARGET_NAME} ${PREFIX_IMPORTED_LIB}libgcc ${PREFIX_IMPORTED_LIB}libgcc_eh)
endif()
${FDLIBM_TARGET_NAME} ${PREFIX_IMPORTED_LIB}libgcc ${PREFIX_IMPORTED_LIB}libgcc_eh)

This comment has been minimized.

@egavrin

egavrin Dec 2, 2015

Contributor

Likewise

@@ -501,7 +519,7 @@ endif()
if(("${PLATFORM}" STREQUAL "DARWIN") AND (NOT (CMAKE_COMPILER_IS_GNUCC OR CMAKE_COMPILER_IS_GNUCXX)))
target_link_libraries(${TARGET_NAME} ${CORE_TARGET_NAME} ${LIBC_TARGET_NAME} ${FDLIBM_TARGET_NAME}
${PREFIX_IMPORTED_LIB}libclang_rt.osx)
else()
else()

This comment has been minimized.

@egavrin

egavrin Dec 2, 2015

Contributor

Likewise

@@ -0,0 +1,103 @@
# Copyright 2014-2015 Samsung Electronics Co., Ltd.

This comment has been minimized.

@egavrin

egavrin Dec 2, 2015

Contributor

Years of copyright are incorrect, should be 2015.

@@ -0,0 +1,4 @@
function sysloop(ticknow) {

This comment has been minimized.

@egavrin

egavrin Dec 2, 2015

Contributor

I'm worrying about the fact that we have different hello-world applications for different platforms. We can improve it later, just to think.

This comment has been minimized.

@seanshpark

seanshpark Dec 2, 2015

Contributor

Yes agree. It's same LED blink example and I want to make them work with one js source code. Maybe move them to some common folder and edit the Makefile for both. I'll work on this after this PR lands.

@@ -0,0 +1,142 @@
#!/usr/bin/env python

This comment has been minimized.

@egavrin

egavrin Dec 2, 2015

Contributor

I accidentaly realised that js2c.py is doing same thing as ./tools/generator.sh. Additionally, ./embedding/mbedk64f/js2c.py and ./embedding/esp8266/js2c.py looks nearly the same.

This comment has been minimized.

@seanshpark

seanshpark Dec 2, 2015

Contributor

Yes with some file path difference. This seems to be done with making common js example code.

@seanshpark seanshpark assigned egavrin and unassigned seanshpark Dec 8, 2015

@seanshpark

This comment has been minimized.

Contributor

seanshpark commented Dec 8, 2015

@egavrin , I think all of your comments are applied :)

@egavrin

This comment has been minimized.

Contributor

egavrin commented Dec 11, 2015

I have only minor question about merging js2c.py and ./tools/generator.sh. Should we merge them, or leave them as is?
Otherwise LGTM

@seanshpark

This comment has been minimized.

Contributor

seanshpark commented Jan 6, 2016

Didn't see the last message :(
about merging js2c.py and ./tools/generator.sh
I think it would be better to merge them when MCU moves to targets folder. But before that, is it ok to change MCU codes to targets style and build structure?

@seanshpark

This comment has been minimized.

Contributor

seanshpark commented Jan 6, 2016

Waiting for one more LGTM :)

buf = ' ' * indent
buf += content
buf += '\n'
fo.write(buf)

This comment has been minimized.

@galpeter

galpeter Jan 6, 2016

Contributor

Why don't we simply write out everything directly? Do we really need to build up a string with + ?

This comment has been minimized.

@seanshpark

seanshpark Jan 13, 2016

Contributor

fix to one line :)

if len(sys.argv) >= 2:
buildtype = sys.argv[1]
fout = open(OUT_PATH + 'jerry_targetjs.h', 'w')

This comment has been minimized.

@galpeter

galpeter Jan 6, 2016

Contributor

Should the fout file object be closed after every write is done?

This comment has been minimized.

@seanshpark

seanshpark Jan 13, 2016

Contributor

fout close is missing, and source file close code = open(path, 'r').read() + '\0' is missing. needs fix.

fout.write(LICENSE);
fout.write(HEADER);
def export_one_file(path, name):

This comment has been minimized.

@galpeter

galpeter Jan 6, 2016

Contributor

In this file we are mixing name conventions for methods, I would prefer to use one.

This comment has been minimized.

@seanshpark

seanshpark Jan 13, 2016

Contributor

yes, agree

fout.write('const static char ' + name + '_n[] = "' + name + '";\n')
fout.write('const static char ' + name + '_s[] =\n{\n')
code = open(path, 'r').read() + '\0'

This comment has been minimized.

@galpeter

galpeter Jan 6, 2016

Contributor

same as fout the file is not closed at all. If we are certain that the python version is at least 2.6 in all cases, then this could be written like this:

with open(path, 'r') as code_file:
    code = code_file.read() + '\0'

BTW: do we really need the '\0'?

This comment has been minimized.

@wateret

wateret Jan 13, 2016

Contributor

@galpeter I think we need '\0' because we are building a character array like 'a', 'b', 'c', ... not "abc..."

static char buffer[128];
int length, maxlength;
length = -jerry_api_string_to_char_buffer (args_p[0].v_string, NULL, 0);
maxlength = MAX(length, 126);

This comment has been minimized.

@galpeter

galpeter Jan 6, 2016

Contributor

If the length is bigger than 128 then the max value will be that number and then we'll be copying more than the size of the buffer into the array, is that really correct? Shouldn't this be MIN instead?

This comment has been minimized.

@seanshpark

seanshpark Jan 13, 2016

Contributor

yes, it should be MIN.

@galpeter

This comment has been minimized.

Contributor

galpeter commented Jan 6, 2016

General question: do we plan to have the same Jerry code style requirements for the files under the targets dir?

@@ -0,0 +1,165 @@
#!/bin/bash

This comment has been minimized.

@galpeter

galpeter Jan 6, 2016

Contributor

Shouldn't this file have a copyright info?

This comment has been minimized.

@seanshpark

seanshpark Jan 13, 2016

Contributor

it's from the esp8266 code and there is no copyright info and no changes.

echo " 3=2048KB( 512KB+ 512KB)"
echo " 4=4096KB( 512KB+ 512KB)"
echo " 5=2048KB(1024KB+1024KB)"
echo " 6=4096KB(1024KB+1024KB)"

This comment has been minimized.

@galpeter

galpeter Jan 6, 2016

Contributor

Is the '1024+1024' correct here?
Ahh.. I see ignore my comment

@@ -0,0 +1,142 @@
#!/usr/bin/env python

This comment has been minimized.

@galpeter

galpeter Jan 6, 2016

Contributor

It seems that this file is present in both targets. Do we really need it twice? Is it somehow target dependent?

This comment has been minimized.

@seanshpark

seanshpark Jan 13, 2016

Contributor

js2c.py is duplicate and somehow I will merge it to one after this PR :)

@seanshpark

This comment has been minimized.

Contributor

seanshpark commented Jan 13, 2016

General question: do we plan to have the same Jerry code style requirements for the files under the targets dir?

Files from the target provider may or should co-exist and it can have its own code style. I think it would be comfortable to leave it as by targets style.

@seanshpark

This comment has been minimized.

Contributor

seanshpark commented Jan 13, 2016

@galpeter , please review again when comfortable.

@galpeter

This comment has been minimized.

Contributor

galpeter commented Jan 13, 2016

seanshpark added some commits Nov 30, 2015

Add external compile flags and entry file for target board builds
JerryScript-DCO-1.0-Signed-off-by: SaeHie Park saehie.park@samsung.com
Add target build for mbed / FRDM-K64F board.
JerryScript-DCO-1.0-Signed-off-by: SaeHie Park saehie.park@samsung.com
Workaround fix for Xtensa (ESP8266) memory alignment exception error
Related issue: #675

JerryScript-DCO-1.0-Signed-off-by: SaeHie Park saehie.park@samsung.com
Add target build for ESP8266 board.
JerryScript-DCO-1.0-Signed-off-by: SaeHie Park saehie.park@samsung.com

@seanshpark seanshpark merged commit feb27a5 into master Jan 13, 2016

@seanshpark seanshpark deleted the embedding branch Jan 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment