Skip to content

Add several options to the nuttx target in precommit.py #1207

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

Merged
merged 1 commit into from
Sep 26, 2017

Conversation

robertsipka
Copy link
Contributor

As @rtakacs mentioned earlier, the precommit.py script should be extended with a flash mechanism.
It is a small modifications, and does not contain to many things to significantly influences the readability and the maintenance of the script.
The other hand, by adding the memstat patches and the testsuite to the binary, we finally able to measure the memory consumption of the IoT.js testsuite on the nuttx target.

@hs0225
Copy link
Contributor

hs0225 commented Sep 21, 2017

@robertsipka Some code seem to duplicate with js-remote-test. Do you plan to merge them?

@robertsipka
Copy link
Contributor Author

@hs0225 : This is the first patch of the other approach, where we could test our development on devices without downloading the js-remote-test project. Several related code parts are already implemented in the js-remote-test, which I have planned to use. The main goal is the testrunner itself had to be part of the IoT.js project.

@hs0225
Copy link
Contributor

hs0225 commented Sep 22, 2017

@robertsipka Are there too many duplicate codes like *.patch in iot.js and jerryscript repository? I think it seems to be the same except the application build.

@robertsipka
Copy link
Contributor Author

@hs0225 : These patches only needed for IoT.js. The reason is that the jerryscript allocator could able to follow the libtuv and IoT.js memory allocations. The jerryscript itself does not have to contain them.

@robertsipka robertsipka force-pushed the build_target branch 3 times, most recently from 7cf71f1 to cd46f31 Compare September 22, 2017 12:03
@rtakacs
Copy link
Contributor

rtakacs commented Sep 22, 2017

@robertsipka I think, it would be a nicer solution if all your patch files (tools/testrunner/patches) could be in a target/nuttx/stm32f4dis/patches folder. The patches are belong logically to the NuttX target, and not to the testrunner. Especially the nuttx-7.19.diff, because that has some bugfixes for the our used NuttX version.

Just a note: the target/nuttx/stm32f4dis/ folder is already has a patch, so all stm32f4dis-nuttx patches could be in one place.

@hs0225
Copy link
Contributor

hs0225 commented Sep 25, 2017

@robertsipka OK got it. @rtakacs I agree with you.

Apply memstat patches to measure the memory consumption of IoT.js.
Create a ROMFS image from the contents of the IoT.js testsuite.
Flash the NuttX binary onto the board.

IoT.js-DCO-1.0-Signed-off-by: Robert Sipka [email protected]
@robertsipka
Copy link
Contributor Author

robertsipka commented Sep 25, 2017

I think we will need these patches (3 of a 4) also in case of artik05x boards, which will require a common folder. On the other hand, since this PR only contain NuttX specific modifications, I agree with you guys and we should move it under a common folder later, if needed (maybe not under the tools/testrunner dir, maybe somewhere under the config dir). I've updated this PR accordingly.

@rtakacs
Copy link
Contributor

rtakacs commented Sep 25, 2017

LGTM (informally)

By the way, could you give an example that shows the usage of your development?

@robertsipka
Copy link
Contributor Author

Adding the memstat patches and the testsuite to the binary for measure the memory consumption of the IoT.js testsuite on the nuttx target:
./tools/precommit.py --test="nuttx" --buildtype=release --buildoptions=--jerry-memstat --enable-testsuite --flash


/* Zero out all members. */
- memset (&JERRY_CONTEXT (JERRY_CONTEXT_FIRST_MEMBER), 0, sizeof (jerry_context_t));
+ // memset (&JERRY_CONTEXT (JERRY_CONTEXT_FIRST_MEMBER), 0, sizeof (jerry_context_t));
Copy link
Member

Choose a reason for hiding this comment

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

Why this is commented out?

Copy link
Contributor

@LaszloLango LaszloLango Sep 25, 2017

Choose a reason for hiding this comment

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

It needs to prevent the overflow of the counter. When jerry_init called, the allocated memory is not zero, because the IoT.js did some allocation for the environment. So we should not zero out memory statistics here, because it will cause an overflow after the free function calls.

Copy link
Contributor

@hs0225 hs0225 left a comment

Choose a reason for hiding this comment

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

LGTM

@yichoi
Copy link
Contributor

yichoi commented Sep 26, 2017

LGTM

@yichoi yichoi merged commit b494fc4 into jerryscript-project:master Sep 26, 2017
robertsipka added a commit to robertsipka/iotjs that referenced this pull request Sep 26, 2017
…uttx target.

Using `.config.alloptions` since it contains all the features that IoT.js requires.
Restored `.config.travis` to the original state (modified in jerryscript-project#1207).

IoT.js-DCO-1.0-Signed-off-by: Robert Sipka [email protected]
robertsipka added a commit that referenced this pull request Sep 27, 2017
…uttx target. (#1219)

Using `.config.alloptions` since it contains all the features that IoT.js requires.
Restored `.config.travis` to the original state (modified in #1207).

IoT.js-DCO-1.0-Signed-off-by: Robert Sipka [email protected]
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.

6 participants