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 support for mbed OS 5.1 #1318

Merged
merged 1 commit into from Sep 21, 2016

Conversation

7 participants
@matthewelse
Copy link
Contributor

commented Sep 2, 2016

Added target for the recently released mbed OS 5.1. So far we've tested JerryScript on the boards listed in targets/mbedos5/README.md (though it should work on the various other boards supported by mbed OS 5.1), and instructions are given in that README about how to build it.

cc: @thegecko @stephenkyle-ARM @janjongboom

@zherczeg

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2016

Wow this is a huge amount of work! Am I see right that the code is in C++ and follows the style of mbed? What is the purpose of .mbedignore?

@LaszloLango

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2016

Is it possible to generate the pin_defs directory? Not sure they should be in jerryscript repository.

@matthewelse

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2016

@zherczeg the purpose of .mbedignore is to allow us to build the jerryscript sources using mbed OS 5's build tool 'mbed cli', without building things like jerry-main and jerry-libc.

@LaszloLango yes, it is possible to generate pin_defs (it follows the same directory structure as the mbed HAL) and there's a script that @stephenkyle-ARM wrote to generate it. I imagine that we could integrate it into the Makefile, though it would probably slow down the initial build quite considerably.

@matthewelse matthewelse force-pushed the ARMmbed:mbed-os-5 branch 2 times, most recently from e9ec9a0 to cf3d478 Sep 5, 2016

@zherczeg

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2016

The truth is I am not in a favour of adding 100+ generated C files to the project. It would be great to figure something out for getting them (generating? downloading as sub-repository?)

@janjongboom

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2016

@matthewelse We don't need to generate every pindef file when we move it to the Makefile right, can only generate it when building for a new target.

@stephenkyle-ARM

This comment has been minimized.

Copy link

commented Sep 6, 2016

Those pin_defs files were generated from the mbed HAL PinNames descriptions: https://github.com/ARMmbed/mbed-os/tree/master/hal/targets/hal

As @matthewelse says, I have a script that generates the pin_defs files, however it depends on using libclang and its python bindings for parsing C++. I haven't checked all the PinNames.h files to see if they could easily be parsed using something more hand-written, but that would probably be the way to go if we can't just include all the generated pins.js files in this project.

@janjongboom

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2016

@stephenkyle-ARM Can we put them in a separate repo and then pull it in same way we pull in mbed-os via make getlibs? It'll be a PITA when we need to open a new PR against jerryscript every time a new target is added in mbed-os repo.

@matthewelse

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2016

I've got a small update coming soon, which should fix a few issues I was having where jerry_parse would crash with non-trivial examples due to a lack of stack size. Howeber, I need to wait for ARMmbed/mbed-os#2646 to be merged first.

As for the target-specific (auto-generated) js code, I agree that it would be nice to be able to avoid having to make a new pull request every time we want to add support for a new target, If there's any way we can reduce the complexity of dependencies required (i.e. preferably no libclang if possible), it'd be nice to generate pins.js on the fly as an additional build step.

@matthewelse matthewelse force-pushed the ARMmbed:mbed-os-5 branch from c0990ef to 2cd50ba Sep 13, 2016

@matthewelse

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2016

We now have legal approval to open source the port, so the DCO is now signed, and squashed.

CC: @janjongboom @thegecko @stephenkyle-ARM

@matthewelse matthewelse force-pushed the ARMmbed:mbed-os-5 branch 4 times, most recently from 19c9daf to 5965621 Sep 13, 2016

"target.uart_hwfc": 0
}
}
}

This comment has been minimized.

Copy link
@LaszloLango

LaszloLango Sep 13, 2016

Contributor

Please add a new line at the end of the file

This comment has been minimized.

Copy link
@stephenkyle-ARM

stephenkyle-ARM Sep 13, 2016

Perhaps we shouldn't include an mbed_app.json file anyway, @matthewelse ?

This comment has been minimized.

Copy link
@matthewelse

matthewelse Sep 13, 2016

Author Contributor

Only reason I included it was to make sure that standard I/O on the NRF52 doesn't cause the program to hang if you aren't connected to the serial port (seems like hanging is the default behaviour in mbed OS for some reason)

@matthewelse matthewelse force-pushed the ARMmbed:mbed-os-5 branch 4 times, most recently from fc80cdd to 1589fd7 Sep 13, 2016

@akosthekiss
Copy link
Contributor

left a comment

I've spotted some licensing issues, mentioned above.

In addition to that, I second @zherczeg 's opinion that houndred+ generated files should not be included in the repository. Is there any solution upcoming for that?

@@ -0,0 +1,72 @@
/* Copyright (c) 2016 ARM Limited. All rights reserved. */

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

Missing license

This comment has been minimized.

Copy link
@matthewelse

matthewelse Sep 15, 2016

Author Contributor

Fixed in d125e2c

@@ -0,0 +1,11 @@
/* Copyright (c) 2016 ARM Limited. All rights reserved. */

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

Missing license

This comment has been minimized.

Copy link
@matthewelse

matthewelse Sep 15, 2016

Author Contributor

Fixed in d125e2c

@@ -0,0 +1,13 @@
/* Copyright (c) 2016 ARM Limited. All rights reserved. */

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

Missing license

This comment has been minimized.

Copy link
@matthewelse

matthewelse Sep 15, 2016

Author Contributor

Fixed in d125e2c

@@ -0,0 +1,84 @@
/* Copyright (c) 2016 ARM Limited. All rights reserved. */

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

Missing license

This comment has been minimized.

Copy link
@matthewelse

matthewelse Sep 15, 2016

Author Contributor

Fixed in d125e2c

#!/usr/bin/env python

# Copyright 2015-2016 Samsung Electronics Co., Ltd.
# Copyright (c) 2016 ARM Limited. All rights reserved.

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

I'm not sure that the "all rights reserved" clause in the copyright line is compatible with the rest of the license.

This comment has been minimized.

Copy link
@matthewelse

matthewelse Sep 15, 2016

Author Contributor

Fixed in d125e2c

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

# Copyright (c) 2016 ARM Limited. All rights reserved.

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

Same here: all rights reserved vs Apache 2.0

This comment has been minimized.

Copy link
@matthewelse

matthewelse Sep 15, 2016

Author Contributor

Fixed in d125e2c

@@ -0,0 +1,94 @@
/* Copyright 2014-2015 Samsung Electronics Co., Ltd.
* Copyright (c) 2016 ARM Limited. All rights reserved.

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

Same here: all rights reserved vs Apache 2.0

This comment has been minimized.

Copy link
@matthewelse

matthewelse Sep 15, 2016

Author Contributor

Fixed in d125e2c

@akosthekiss

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2016

A comment/idea about the pindefs: As it is heavily dependent on the mbed-os module/library, it might not be a good idea to pre-generate and store them (like, anywhere; neither here, nor at an external place). If and when mbed-os gets any maintenance changes or extensions, it will be error-prone to keep them in sync. On the other hand, I understand that running the previously mentioned script that generates the js/pin_defs directory may take a long time to complete. However (and here comes the core message/idea): why do we have to generate all pins.js files when we build for a single given target? Cannot we have a modified version of that script that generates one pins.js and that's it? (Note: I also understand that it has dependencies that jerryscript itself has not (libclang and python bindings). However, the whole target has additional dependencies, even in terms of tools, e.g., the mbed cli, so this should not be a blocking point.)

An additonal note: I have the impression that the js2c.py file in this target does a bit more than its name suggests. It mixes the selection of the correct pins.js and creates the .c file with the array from all .js files (pins.js included). If there would be something like a gen_pins_js.py (see above), then js2c.py could be kept "clean" and kept in sync with js2c.py used by other targets (to avoid copy-pasting, cloning, and promote code reuse).

FNULL = open(os.devnull, 'w')

try:
cmd = "dir /s /b /AD \"{}\"".format("TARGET_" + args.mcu)

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 15, 2016

Contributor

I'm not sure that it's really a good idea to have a separate windows/linux cmd for searching for file. We can do this directly in python no need to call out to the system.

print('\n'.join(js_target_list))
sys.exit(1)
else:
os.system("cp {} ./js/pins.js".format(js_target_list[0]))

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 15, 2016

Contributor

Python has good support for file operations, please check the shutil module.


try:
cmd = "dir /s /b /AD \"{}\"".format("TARGET_" + args.mcu)
intended_target = subprocess.check_output(cmd, shell = True, cwd=SRC_PATH, stderr=FNULL).strip()

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 15, 2016

Contributor

Calling the subprocess methods with shell=True is not advised.

print("JSHint picked up an undefined variable in your code.")
print("Are you using a PinName that doesn't exist for your platform?")
raise
sys.exit(1)

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 15, 2016

Contributor

I don't think that the sys.exit will be ever called if we re-raise the exception before.

def exportOneFile(path, name):
# Prepend pins.js onto main.js, don't include pins.js itself.
code = ""
if name == "pins":

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 15, 2016

Contributor

I would not put the pin name/value extraction here as it clearly not the same as the methods original purpose. Simply add a check where the method is called and do the extraction there (preferably as a method).

exportOneName('main')
filenames = sorted(map(extractName, files), key=str.lower)
for name in filenames:
if name != 'main' and name != 'pins':

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 15, 2016

Contributor

Better way: if name not in ['main', 'pins']:

writeLine(fout, 'const char *jsmbed_js_magic_strings[] = {')
comma = ","

for (idx, (pinname, value)) in enumerate(pins):

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 15, 2016

Contributor

I'm not a big fan of iterating over the same thing more than one times. We could extract the required data in one go. Then write it out when we need it. Also adding the comma manually (basically joining strings) makes me wondering if I can do it better.

In short, maybe something like this can be useful:

string_entries = []
length_entries = []
value_entries = []

for (idx, (name, value)) in enaumerate(pins):
    string_entries.append('"{}"'.format(name))
    length_entries.append('{}'.format(len(name)))
    value_entries.append('{}'.format(value))


writeLine(fout, ',\n    '.join(string_entries), 1)
...
writeLine(fout, ',\n    '.join(length_entries), 1)
...
writeLine(fout, ',\n    '.join(value_entries), 1)
@@ -0,0 +1,276 @@
#!/usr/bin/env python

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 15, 2016

Contributor

I'm have the same view as @akosthekiss, the pin js file generation and extraction should be done in a different script and not in the js2c.py.

@matthewelse

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2016

@galpeter I'm currently working on a new python script to generate pins.js on the fly, so that we can make use of the common js2c.py tool. As such, it's worth waiting for the time being before we do a thorough code review.

@akosthekiss

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2016

@matthewelse, just a quick note: if it turns out that the common js2c.py does not quite suite your needs, or could be improved (not in a way that would mix in unrelated functionality but so that it could perform its task better, in a more generic or adaptable way, with less hard-coded stuff, etc), do propose changes to it. That might be useful for other targets as well.

@matthewelse matthewelse force-pushed the ARMmbed:mbed-os-5 branch from d125e2c to 1f3f152 Sep 15, 2016

#include "jerryscript-mbed-drivers/InterruptIn-js.h"
#include "jerryscript-mbed-drivers/DigitalOut-js.h"
#include "jerryscript-mbed-drivers/setInterval-js.h"
#include "jerryscript-mbed-drivers/setTimeout-js.h"

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

Any reason for this extra space?



DECLARE_CLASS_FUNCTION(DigitalOut, write) {
// check and unbox arguments

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

Most of the target code seems to use 2-space indentation, but this file uses 4 spaces. It would be nice to use a consistent style throughout the target.

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

Another style inconsistency is the placement of braces (together with declaration or on separate line).

@akosthekiss
Copy link
Contributor

left a comment

First round of review on the updated PR. Some comments related to style, some related to the use of jerry API, some are more general.

And one more (couldn't put a line comment in an empty file): what is tools/cmsis.h for? It's empty, and couldn't see it used.

ATTACH_CLASS_FUNCTION(js_object, InterruptIn, disable_irq);

return js_object;
}

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

avoid files without NL at EOF

}



This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

style: too many empty lines

}
else {
LOG_PRINT_ALWAYS("invalid arguments in method write");
return jerry_create_undefined();

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

It might be better perhaps to return an error object with an error message (e.g., with jerry_create_error(JERRY_ERROR_TYPE, "...")), instead of logging and returning undefined.

{
LOG_PRINT_ALWAYS("ERROR: Script assertion failed\n");
exit(1);
return jerry_create_undefined();

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

exit and then return? Looking at the code, the compiler should not complain even if return would be omitted.


BoundCallback(Callback<R(A0)> cb, A0 a0) : a0(a0), cb(cb) { }

void call() {

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

another file with style inconsistencies

static EventLoop instance;

public:
static EventLoop& getInstance() {

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

ditto (style: indent, braces)

static LibraryRegistry instance;

public:
static LibraryRegistry& getInstance() {

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

ditto (style)

Callback<void()> wrapFunction(jerry_value_t f) {
if (!jerry_value_is_function(f)) {
LOG_PRINT_ALWAYS("invalid function passed\r\n");
exit(1);

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

Is this really what we want? That exit signals some bad smell to me. Shouldn't these four lines just be replaced with an assert(jerry_value_is_function(f));, or something similar?

@@ -0,0 +1,214 @@
"""

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 15, 2016

Contributor

Missing Apache 2.0 license header

@matthewelse

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2016

@galpeter and @akosthekiss - I think the most recent commits should address the suggestions you had.

@akosthekiss
Copy link
Contributor

left a comment

Second round of comments added.
I still miss the explanation for the empty tools/cmsis.h f file.

MBED_CLI_FLAGS += -t GCC_ARM

.PHONY: all js2c getlibs rebuild library pins
all: source/jerry_target.h source/pins.cpp .mbed ../../.mbedignore

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 16, 2016

Contributor

source/jerry_target.h is bad reference. source/jerry_targetjs.h might be better

mbed target $(BOARD)
mbed compile $(MBED_CLI_FLAGS) --library

rebuild: clean all

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 16, 2016

Contributor

This is dangerous. AFAIK, make makes no guarantees about the order of the execution of the dependencies. Especially if you run it with -j, then all and clean will happen in parallel.

}

LOG_PRINT_ALWAYS("ERROR: Unexpected number of arguments to I2C.read, expected 1, 3 or 4.");
return jerry_create_undefined();

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 16, 2016

Contributor

a jerry_create_error would fit here as well

@@ -0,0 +1 @@
https://github.com/ARMmbed/mbed-events/#a4ba1b08ef90e2b3b6d442696f4d80a8587494e3

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 16, 2016

Contributor

According to https://docs.mbed.com/docs/mbed-os-handbook/en/5.1/dev_tools/cli/, .lib files are not to be hand-managed: "Warning: mbed CLI stores information about libraries and dependencies in reference files that use the .lib extension (like lib_name.lib). While these files are human-readable, we strongly advise that you don’t edit these manually - let mbed CLI manage them instead." Do we need to check this file in the repository, or should they better be added to .gitignore?

This comment has been minimized.

Copy link
@matthewelse

matthewelse Sep 16, 2016

Author Contributor

These .lib files definitely need to be in source control, otherwise mbed cli doesn't know what libraries to fetch.

@@ -0,0 +1 @@
https://github.com/ARMmbed/mbed-os/#bdab10dc0f90748b6989c8b577771bb403ca6bd8

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 16, 2016

Contributor

same question for this file

@@ -0,0 +1,226 @@
"""
getpins_js.py

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 16, 2016

Contributor

Name is incorrect. Moreover, I suggest to let this file start just like other python files in the project:

#!/usr/bin/env python

# Copyright 2016 Samsung Electronics Co., Ltd.
# Copyright 2016 University of Szeged.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#     http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

That is,

  • no repetition of the file name, so you wont have to keep it in sync with actual file name,
  • let first line be a hashbang line,
  • put license in comment, not in docstring.

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 16, 2016

Contributor

(of course, with a different copyright notice)

#define CHECK_ARGUMENT_COUNT(CLASS, NAME, EXPR) \
if (!(EXPR)) { \
LOG_PRINT_ALWAYS("ERROR: wrong argument count for %s.%s, expected %s.\n", # CLASS, # NAME, # EXPR ); \
return false; \

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 16, 2016

Contributor

This code is suspicious. This macro is used in functions, which return jerry_value_t. Returning false here (and printing) is most probably not the right way. (But returning with jerry_create_error might be the right thing.)

This comment has been minimized.

Copy link
@matthewelse

matthewelse Sep 16, 2016

Author Contributor

You're correct - this code is a hangover from an older version of the jerryscript API, and GCC is casting the false value to jerry_value_t, which is definitely bad. I'll fix this, and similar macros too.

return jerry_create_undefined();
} else {
const char* error_msg = "Invalid arguments in method write";
return jerry_create_error(JERRY_ERROR_TYPE, reinterpret_cast<const jerry_char_t*>(error_msg));

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 16, 2016

Contributor

BTW, couldn't the CHECK_ARGUMENT_ macros be used in this file's functions as well? (Once their return value is fixed, of course.)

This comment has been minimized.

Copy link
@matthewelse

matthewelse Sep 16, 2016

Author Contributor

@akosthekiss you're right, the only reason it's like this is because DigitalOut.cpp was auto-generated using a script I wrote at some point, though for consistency since we're putting it into source control, I'll modify it to be consistent with the other driver sources.

mbed-events
.build
.mbed
mbed_settings.py

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 16, 2016

Contributor

Cannot find this file, is it needed in .gitignore?

This comment has been minimized.

Copy link
@matthewelse

matthewelse Sep 16, 2016

Author Contributor

mbed_settings.py is created by mbed cli in mbedos5, but is specific to individual users' machines, so shouldn't be under version control.

@matthewelse

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2016

@akosthekiss the empty cmisis.h file is to simplify the process of parsing PinNames.h files in the mbed source tree (prevents a huge number of header files being included)

@akosthekiss

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2016

@matthewelse Then I'd suggest putting a proper content in the cmsis.h, with header + a comment with explanation like /* Intentionally empty replacement header for the purposes of the generate_pins.py tool */

/**
* DigitalOut#is_connected (native JavaScript method)
*
* @returns 0 if the DigitalOut is set to NC, or 1 if it is connected to an

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 18, 2016

Contributor

Is the 0/1 return value intentional? JS does have a boolean type. Why not return true/false?
(Not insisting though, just asking.)

This comment has been minimized.

Copy link
@matthewelse

matthewelse Sep 18, 2016

Author Contributor

There's no particular reason not to return true/false, this is simply matching what is returned by the mbed API.

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 18, 2016

Contributor

OK with me to keep it integer then

}

const char* error_msg = "ERROR: Unexpected number of arguments to I2C.read, expected 1, 3 or 4.";
return jerry_create_error(JERRY_ERROR_TYPE, reinterpret_cast<const jerry_char_t*>(error_msg));

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 18, 2016

Contributor

Couldn't this be replaced with CHECK_ARGUMENT_COUNT(I2C, read, (args_count == 1 || args_count == 3 || args_count == 4));, moved to the start of the function?

*/
DECLARE_CLASS_FUNCTION(I2C, frequency) {
CHECK_ARGUMENT_COUNT(Ticker, frequency, (args_count == 1));
CHECK_ARGUMENT_TYPE_ALWAYS(Ticker, frequency, 0, number);

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 18, 2016

Contributor

Ticker is not right here I guess, should be I2C.

}

const char* error_msg = "ERROR: Unexpected number of arguments to I2C.write, expected 1, 3 or 4.";
return jerry_create_error(JERRY_ERROR_TYPE, reinterpret_cast<const jerry_char_t*>(error_msg));

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 18, 2016

Contributor

(same macro approach here as for I2C.read)

jsmbed_wrap_register_global_function(const char* name,
jerry_external_handler_t handler)
{
jerry_value_t global_object_val = jerry_get_global_object();

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 18, 2016

Contributor

(style) This single file seems to have retained the 2-space indentation. Stick to one style.

@@ -0,0 +1,4 @@
/*

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 18, 2016

Contributor

This file only got a body, not a license header, as requested. Please add a proper Apache 2.0 license header.

#!/usr/bin/env python

# Copyright (c) 2016 ARM Limited

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 18, 2016

Contributor

This is not exactly the header found in other files and suggested in previous review. The missing # seems like a minor technicality, but most probably we will have an automated license checker set up soon, and it will choke on such differences.

clean:
rm -rf .build/$(BOARD)

js2c: js/main.js js/flash_leds.js pins

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 18, 2016

Contributor

Why is pins a dependency if pins.js is ignored? (BTW, why is pins.js ignored, will it be not needed to declare the LED1..4 variables?)

This comment has been minimized.

Copy link
@matthewelse

matthewelse Sep 18, 2016

Author Contributor

pins.js is ignored, since it is incorporated into the binary as an array of magic strings, to reduce memory consumption.

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 18, 2016

Contributor

Q1: Is jshint the only reason for pins.js then?
Q2: Isn't pins a superfluous dependency for js2c then?

This comment has been minimized.

Copy link
@matthewelse

matthewelse Sep 19, 2016

Author Contributor

Q1: yes
Q2: yes (fixed in soon to be pushed commit)

@matthewelse matthewelse force-pushed the ARMmbed:mbed-os-5 branch from ce60eed to 4fa929f Sep 19, 2016

@akosthekiss
Copy link
Contributor

left a comment

I think we are getting close. I'm running out of comments. These are hopefully my final comments before approval.

@@ -21,8 +21,12 @@
import os
import re

import argparse

special_chars = re.compile('[-|\\\\|\\?|\\\'|".]')

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 20, 2016

Contributor

This regex does not seem right. Within character classes, you don't need alternation (actually, special characters lose their meaning). And using raw strings could help to get rid of lots of backspace escaping. A quick try in python shell gave:

>>> special_chars = re.compile(r'[-\\?\'".]')
>>> special_chars.sub('_', "x-x?x'x\"x.x\\x")
'x_x_x_x_x_x_x'

(If you want to replace dash, backslash, question mark, single and double quotes, and period.)


if (jerry_value_has_error_flag(set_result)) {
is_ok = false;
LOG_PRINT_ALWAYS("Error: register_native_function failed: [%s]\r\n", name);

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 20, 2016

Contributor

Not sure what"register_native_function" is referring to. This function is named "jsmbed_wrap_register_global_function", perhaps that was intended.

if (!(jerry_value_is_function(reg_function)
&& jerry_value_is_constructor(reg_function))) {
is_ok = false;
LOG_PRINT_ALWAYS("Error: create_external_function failed !!!\r\n");

This comment has been minimized.

Copy link
@akosthekiss

akosthekiss Sep 20, 2016

Contributor

Also a bit misleading error message missing the "jerry_" prefix.

(BTW: Any intention to get error messages even more consistent? Current strings use "ERROR" vs "Error" and "!!!" vs "!" vs "." vs nothing and "\r\n" vs "\n\r" vs "\n".)

@akosthekiss

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2016

(One technical note: right now, there are 30 patches on top of each other. Before landing can happen, it would be nice to have all changes squashed, amended with a consolidated commit message, and rebased on latest master. Actually, we have everything in place to do that by the final reviewer/maintainer, but with 30 patches, it'd be better if you could do that, not to get anything wrong.)

@matthewelse matthewelse force-pushed the ARMmbed:mbed-os-5 branch from 292b491 to 16c8de3 Sep 21, 2016

@matthewelse

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2016

@akosthekiss AFAICT this should resolve the last few issues you had. Thanks for the code review - the code is in much better shape thanks to your help 👍.

I've also gone ahead and squashed the commits together.

@matthewelse matthewelse force-pushed the ARMmbed:mbed-os-5 branch from 16c8de3 to 69566e1 Sep 21, 2016

@akosthekiss

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2016

From my side, we have reached a fine situation. This port can act as a good example for users of JerryScript, IMHO.
(There are still some subtleties that I could point out, especially around error reporting, bot nothing blocker. If I find time for it, I may even open some PRs for them.)
LGTM

#include "jerryscript-mbed-library-registry/registry.h"

mbed::js::LibraryRegistry mbed::js::LibraryRegistry::instance;

This comment has been minimized.

Copy link
@zherczeg

zherczeg Sep 21, 2016

Contributor

Two newlines is intentional?

This comment has been minimized.

Copy link
@matthewelse

matthewelse Sep 21, 2016

Author Contributor

Nope. Fixed now


for root, dirs, files in os.walk(root_dir, topdown=True):
# modify dirs in place
dirs[:] = filter(lambda x: x in directory_labels or not x.startswith('TARGET_'), dirs)

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 21, 2016

Contributor

The directory_labels here is not passed as an argument, do we really want to use a global variable here?

This comment has been minimized.

Copy link
@matthewelse

matthewelse Sep 21, 2016

Author Contributor

That was an accident - fixed it now

jerry_set_property(this_obj, property_name, handler_obj);

// TODO: check whether these are needed!
jerry_release_value(handler_obj);

This comment has been minimized.

Copy link
@zherczeg

zherczeg Sep 21, 2016

Contributor

They are needed.

This comment has been minimized.

Copy link
@matthewelse

matthewelse Sep 21, 2016

Author Contributor

👍 Removed that comment now

@zherczeg

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2016

I am also happy with the current patch. LGTM

"""
for root, dirs, files in os.walk(root_dir, topdown=True):
# modify dirs in place
dirs[:] = filter(lambda x: x in directory_labels

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 21, 2016

Contributor

The directory_labels here is not passed as an argument, do we really want to use a global variable here?

Filter out directories that are not in directories, or do not start with
TARGET_.
Since this looks in (essentially )the same directories as the compiler would

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 21, 2016

Contributor

typo: space before the closing parenthesis and not after.


LICENSE = '''/* Copyright 2016 ARM, Ltd.
*
* Licensed under the Apache License, Version 2.0 (the \"License\");

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 21, 2016

Contributor

AFAIK there is no need to escape the " characters.

};
''' % MAGIC_STRINGS

args.c.write(LICENSE + COUNT + LENGTHS_SOURCE + MAGIC_SOURCE + MAGIC_STRING_SOURCE)

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 21, 2016

Contributor

There is no need to concatenate each string into one before writing it out. We should write out each one with a separate call.

parser = argparse.ArgumentParser(description=description)

parser.add_argument('board', help='mbed board name')
parser.add_argument('-o',

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 21, 2016

Contributor

A friendlier name for the output files would be better imho. The args.o does not tell much by itself.

Enumerate pins specified in PinNames.h, by looking for a PinName enum
typedef somewhere in the file.
"""
definitions += ['__attribute(x)__=', '__extension__(x)=', 'register=', '__IO=', 'uint32_t=unsigned int']

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 21, 2016

Contributor

Could we merge this list with the gcc_args below? That way we don't need to modify the definitions list and also no need for the extra list copy before calling this method.

# enumerate pins from PinNames.h
pins = enumerate_pins(pins_file, ['./tools'] + list(includes), defines)

out_file = '\r\n'.join(['var %s = %s;' % pin for pin in pins.iteritems()])

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 21, 2016

Contributor

We should use items instead of iteritems as in python3 the iter one was removed. Just to be a bit future proof. This applies to the other iter* methods on lists/dicts.

parser = argparse.ArgumentParser(description="js2c")
parser.add_argument('build_type', help='build type', default='release', nargs='?')
parser.add_argument('--ignore', help='files to ignore', dest='ignore_files', default=[], action='append')
parser.add_argument('--no-main', help='don\'t require a main.js file', dest='main', action='store_false', default=True)

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 21, 2016

Contributor

You can eliminate the need for escaping the ' character if the string starts with the " character (and also ends with it): "don't require..."

@@ -77,10 +81,16 @@ def removeWhitespaces(code):
OUT_PATH = './source/'
SRC_PATH = './js/'

parser = argparse.ArgumentParser(description="js2c")
parser.add_argument('build_type', help='build type', default='release', nargs='?')

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 21, 2016

Contributor

We can restrict the choices to release and debug if we provide the choices list for the argument.

parser = argparse.ArgumentParser(description="js2c")
parser.add_argument('build_type', help='build type', default='release', nargs='?')
parser.add_argument('--ignore', help='files to ignore', dest='ignore_files', default=[], action='append')
parser.add_argument('--no-main', help='don\'t require a main.js file', dest='main', action='store_false', default=True)

This comment has been minimized.

Copy link
@galpeter

galpeter Sep 21, 2016

Contributor

Maybe a better name for the variable would be: use_main?

Matthew Else Matthew Else
Initial support for mbed OS 5.1
- Wrappers for mbed I/O drivers
- Makefile for automating build process
- Script to generate pin definitions from mbed OS source tree
- Updates to js2c to enable building without a main.js file

JerryScript-DCO-1.0-Signed-off-by: Matthew Else Matthew.Else@arm.com

@matthewelse matthewelse force-pushed the ARMmbed:mbed-os-5 branch from 69566e1 to 577c7f4 Sep 21, 2016

@matthewelse

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2016

@zherczeg @galpeter @akosthekiss Once again, thank you all for your comments.

@galpeter
Copy link
Contributor

left a comment

Did a quick check on the python parts. Nothing breaking thing there, could be done as follow-ups imho.

@akosthekiss

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2016

@matthewelse do you plan any more updates to the pr or is it settled now?

@matthewelse

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2016

@akosthekiss This is it, certainly - any additional features can come in future PRs :)

@akosthekiss akosthekiss merged commit 454d3af into pando-project:master Sep 21, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.