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
Merged

Conversation

matthewelse
Copy link
Contributor

@matthewelse matthewelse 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

@LaszloLango LaszloLango added development jerry-port labels Sep 2, 2016
@zherczeg
Copy link
Member

@zherczeg zherczeg 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
Copy link
Contributor

@LaszloLango LaszloLango commented Sep 5, 2016

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

@matthewelse
Copy link
Contributor Author

@matthewelse matthewelse 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 mbed-os-5 branch 2 times, most recently from e9ec9a0 to cf3d478 Compare Sep 5, 2016
@zherczeg
Copy link
Member

@zherczeg zherczeg 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
Copy link
Contributor

@janjongboom janjongboom 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
Copy link

@stephenkyle-ARM stephenkyle-ARM 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
Copy link
Contributor

@janjongboom janjongboom 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
Copy link
Contributor Author

@matthewelse matthewelse 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
Copy link
Contributor Author

@matthewelse matthewelse 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 mbed-os-5 branch 4 times, most recently from 19c9daf to 5965621 Compare Sep 13, 2016
"target.uart_hwfc": 0
}
}
}
Copy link
Contributor

@LaszloLango LaszloLango Sep 13, 2016

Choose a reason for hiding this comment

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

Please add a new line at the end of the file

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@matthewelse matthewelse Sep 13, 2016

Choose a reason for hiding this comment

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

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 mbed-os-5 branch 4 times, most recently from fc80cdd to 1589fd7 Compare Sep 14, 2016
Copy link
Member

@akosthekiss akosthekiss 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. */
Copy link
Member

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

Missing license

Copy link
Contributor Author

@matthewelse matthewelse Sep 15, 2016

Choose a reason for hiding this comment

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

Fixed in d125e2c

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

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

Missing license

Copy link
Contributor Author

@matthewelse matthewelse Sep 15, 2016

Choose a reason for hiding this comment

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

Fixed in d125e2c

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

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

Missing license

Copy link
Contributor Author

@matthewelse matthewelse Sep 15, 2016

Choose a reason for hiding this comment

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

Fixed in d125e2c

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

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

Missing license

Copy link
Contributor Author

@matthewelse matthewelse Sep 15, 2016

Choose a reason for hiding this comment

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

Fixed in d125e2c

#!/usr/bin/env python

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

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@matthewelse matthewelse Sep 15, 2016

Choose a reason for hiding this comment

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

Fixed in d125e2c

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

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

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

Same here: all rights reserved vs Apache 2.0

Copy link
Contributor Author

@matthewelse matthewelse Sep 15, 2016

Choose a reason for hiding this comment

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

Fixed in d125e2c

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

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

Same here: all rights reserved vs Apache 2.0

Copy link
Contributor Author

@matthewelse matthewelse Sep 15, 2016

Choose a reason for hiding this comment

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

Fixed in d125e2c

@akosthekiss
Copy link
Member

@akosthekiss akosthekiss 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)
Copy link
Contributor

@galpeter galpeter Sep 15, 2016

Choose a reason for hiding this comment

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

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]))
Copy link
Contributor

@galpeter galpeter Sep 15, 2016

Choose a reason for hiding this comment

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

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()
Copy link
Contributor

@galpeter galpeter Sep 15, 2016

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

@galpeter galpeter Sep 15, 2016

Choose a reason for hiding this comment

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

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":
Copy link
Contributor

@galpeter galpeter Sep 15, 2016

Choose a reason for hiding this comment

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

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':
Copy link
Contributor

@galpeter galpeter Sep 15, 2016

Choose a reason for hiding this comment

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

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

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

for (idx, (pinname, value)) in enumerate(pins):
Copy link
Contributor

@galpeter galpeter Sep 15, 2016

Choose a reason for hiding this comment

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

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
Copy link
Contributor

@galpeter galpeter Sep 15, 2016

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@matthewelse matthewelse 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
Copy link
Member

@akosthekiss akosthekiss 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.

#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"
Copy link
Member

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

Any reason for this extra space?



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

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Member

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

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

Copy link
Member

@akosthekiss akosthekiss 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;
}
Copy link
Member

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

avoid files without NL at EOF

}



Copy link
Member

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

style: too many empty lines

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

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

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();
Copy link
Member

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

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() {
Copy link
Member

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

another file with style inconsistencies

static EventLoop instance;

public:
static EventLoop& getInstance() {
Copy link
Member

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

ditto (style: indent, braces)

static LibraryRegistry instance;

public:
static LibraryRegistry& getInstance() {
Copy link
Member

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

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);
Copy link
Member

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

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 @@
"""
Copy link
Member

@akosthekiss akosthekiss Sep 15, 2016

Choose a reason for hiding this comment

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

Missing Apache 2.0 license header

@matthewelse
Copy link
Contributor Author

@matthewelse matthewelse commented Sep 16, 2016

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

Copy link
Member

@akosthekiss akosthekiss 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
Copy link
Member

@akosthekiss akosthekiss Sep 16, 2016

Choose a reason for hiding this comment

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

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
Copy link
Member

@akosthekiss akosthekiss Sep 16, 2016

Choose a reason for hiding this comment

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

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();
Copy link
Member

@akosthekiss akosthekiss Sep 16, 2016

Choose a reason for hiding this comment

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

a jerry_create_error would fit here as well

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

@akosthekiss akosthekiss Sep 16, 2016

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@matthewelse matthewelse Sep 16, 2016

Choose a reason for hiding this comment

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

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
Copy link
Member

@akosthekiss akosthekiss Sep 16, 2016

Choose a reason for hiding this comment

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

same question for this file

@@ -0,0 +1,226 @@
"""
getpins_js.py
Copy link
Member

@akosthekiss akosthekiss Sep 16, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Member

@akosthekiss akosthekiss Sep 16, 2016

Choose a reason for hiding this comment

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

(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; \
Copy link
Member

@akosthekiss akosthekiss Sep 16, 2016

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor Author

@matthewelse matthewelse Sep 16, 2016

Choose a reason for hiding this comment

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

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));
Copy link
Member

@akosthekiss akosthekiss Sep 16, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@matthewelse matthewelse Sep 16, 2016

Choose a reason for hiding this comment

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

@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
Copy link
Member

@akosthekiss akosthekiss Sep 16, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@matthewelse matthewelse Sep 16, 2016

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@matthewelse matthewelse 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
Copy link
Member

@akosthekiss akosthekiss 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
Copy link
Member

@akosthekiss akosthekiss Sep 18, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@matthewelse matthewelse Sep 18, 2016

Choose a reason for hiding this comment

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

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

Copy link
Member

@akosthekiss akosthekiss Sep 18, 2016

Choose a reason for hiding this comment

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

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));
Copy link
Member

@akosthekiss akosthekiss Sep 18, 2016

Choose a reason for hiding this comment

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

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);
Copy link
Member

@akosthekiss akosthekiss Sep 18, 2016

Choose a reason for hiding this comment

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

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));
Copy link
Member

@akosthekiss akosthekiss Sep 18, 2016

Choose a reason for hiding this comment

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

(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();
Copy link
Member

@akosthekiss akosthekiss Sep 18, 2016

Choose a reason for hiding this comment

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

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

@@ -0,0 +1,4 @@
/*
Copy link
Member

@akosthekiss akosthekiss Sep 18, 2016

Choose a reason for hiding this comment

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

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

Copy link
Member

@akosthekiss akosthekiss Sep 18, 2016

Choose a reason for hiding this comment

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

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
Copy link
Member

@akosthekiss akosthekiss Sep 18, 2016

Choose a reason for hiding this comment

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

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?)

Copy link
Contributor Author

@matthewelse matthewelse Sep 18, 2016

Choose a reason for hiding this comment

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

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

Copy link
Member

@akosthekiss akosthekiss Sep 18, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@matthewelse matthewelse Sep 19, 2016

Choose a reason for hiding this comment

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

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

Copy link
Member

@akosthekiss akosthekiss 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('[-|\\\\|\\?|\\\'|".]')
Copy link
Member

@akosthekiss akosthekiss Sep 20, 2016

Choose a reason for hiding this comment

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

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);
Copy link
Member

@akosthekiss akosthekiss Sep 20, 2016

Choose a reason for hiding this comment

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

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");
Copy link
Member

@akosthekiss akosthekiss Sep 20, 2016

Choose a reason for hiding this comment

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

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
Copy link
Member

@akosthekiss akosthekiss 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
Copy link
Contributor Author

@matthewelse matthewelse 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 mbed-os-5 branch from 16c8de3 to