Feature/rtt support #21

CoqRogue · 2017-11-14T09:58:33Z

Added RTT.

Documentation is not done, will be a separate PR as soon as it is done.

…for jlinkarm location, clockspeed, and device family. Also some interface cleanup for loading libraries. Linux and OS X will not work with this commit due to statically looking up the .dll files for highlevel and nrfjprog library. This will be fixed shortly.

This reverts commit 4c55a28.

This reverts commit 118d2d7.

# Conflicts: # src/highlevel.cpp # src/highlevel.h # src/highlevel_common.h # src/highlevel_helpers.h # src/platform/linux/libraryloader.cpp # src/platform/osx/libraryloader.cpp

Add timestamps to get an idea on when data is produced on the device.

bencefr · 2017-11-14T11:47:10Z

nrfjprog/include/nrfjprog.h

+#define micro_version (1)
+
+
+ enum NrfjprogErrorCodesType {


What's wrong with indentation here?

This is a file included from nRFjprog. No need to review it.

bencefr · 2017-11-14T13:27:21Z

src/rtt_batons.h

+    uint32_t result;
+    nrfjprogdll_err_t lowlevelError;
+
+    uv_work_t *req;


std::unique_ptr would be much safer and simpler.

Will consider it.

bencefr · 2017-11-14T13:31:17Z

src/utility/conversion.cpp

+
+    if (justMicrosecondPart)
+    {
+        duration = duration % 1000;


This seems to be only the least significant 3 decimal digits of the microseconds part.

That is correct. If you want only the microseconds, you call this function with the last parameter set to true. This is mostly for occasions where you might want to indicate the time as x min, yy s, zzz ms, æææ us

I don't see such use-case, and by human understanding I'd expect duration % 1e6 as result.

I agree that the usecase is not there anymore. Will remove this code as well as a couple of conversion functions not needed anymore.

IvanSanchez

Looks as incomprehensible as the existing nrfjprog-js code (which is not a bad thing 😉).

LGTM, specially as there are automated tests 👍

Still needs documentation 😞 but that can be done at a later PR.

IvanSanchez · 2017-11-14T10:23:38Z

nrfjprog/include/nrfjprog.h

+
+ enum NrfjprogErrorCodesType {
+
+	Success                                     = 0,				// Requested operation (operations) were successfully completed.


Check whitespace. Looks funky.

IvanSanchez · 2017-11-14T10:25:46Z

src/nrfjprogwrapper.cpp

+        return errorcode_t::CouldNotLoadDLL;
+    }
+
+    if (!load_func_ptr(&dll_function->dll_version, "NRFJPROG_dll_version", nrfJproglibraryHandle)) {


Since the return value of all these ifs is the same, this might look better as a big chain of newline-separated || statements

It would make it slightly harder to debug as it is harder to set break points. Other than that it is a good idea. Will consider into it.

IvanSanchez · 2017-11-14T10:33:01Z

src/rtt.cpp

+
+void RTT::logCallback(const char *msg)
+{
+    log(msg);


Wait, what does this do? Just call log()? Does it work despite logCallback receiving *msg and log receiving std::string? Couldn't the calls to logCallback just be renamed into log???

😕

IvanSanchez · 2017-11-14T11:53:15Z

src/highlevel.cpp

+    CallFunction(info, p, e, nullptr, true);
+}
+
+#include "rtt.h"


#includes should be hoisted to the top of the file

IvanSanchez · 2017-11-14T13:29:24Z

src/rtt.cpp

+        RETURN_ERROR_ON_FAIL(dll_function.rtt_start(), RTTCouldNotStartRTT);
+
+
+        while (true) {


This loop is polling. Plz consider adding something like sleep(25).

That would sleep 25 seconds.

s/sleep/usleep

IvanSanchez · 2017-11-14T13:30:26Z

src/rtt.cpp

+
+        RETURN_ERROR_ON_FAIL(dll_function.rtt_read_channel_count(&downChannelNumber, &upChannelNumber), RTTCouldNotGetChannelInformation);
+
+        for(uint32_t i = 0; i < downChannelNumber; ++i)


A uint32_t to iterate channel numbers? Are we having more than 65K channels on a RTT? LOL 🤣

uint32_t covers slightly more then 65K, is it not, or what really is the issue? If the external library uses that type to return channel number, then let it be.

This is, as @bencefr also mentions, due to rtt_read_channel_count utilize a uint32_t. To avoid the loops producing compile time warnings, it is recommended to use the same type in the loop.

IvanSanchez · 2017-11-14T13:32:14Z

src/rtt.cpp

+
+    releasenRFjprog();
+
+    libraryLoaded = false;


There might be an edge case / race condition here. Are there any instances where cleanup might occur twice in quick succession (in a shorter time than it takes for the DLL stuff to unload)?

You are right in that race conditions may occur. The current implementation expects that the user is nice and never calls a function before the previous callback was completed. Will consider a fix for this.

Have added an execution mutex that locks with a timeout of 10 seconds on all function calls. All race conditions should be handled by that. Two tests were written to test this, and one of them confirms that the guards work by first calling start, and then call read three times in a row. With previous implementation, the reads would return with an error that rtt was not started. Currently they return without error.

Attempting to hit the 10 second timeout for the mutex proved harder. Even with 1M calls to read in a row as fast as possible, you would never get an error. But I have implemented the same functionality for nRFjprog, and when I attempt to program multiple times as fast as possible, the timeout triggers. This can be seen in __tests__/racecondition.test.js

bencefr · 2017-11-14T13:48:51Z

src/rtt_batons.h

+#include "rtt.h"
+#include "rtt_helpers.h"
+
+#define RTTBATON_CONSTRUCTOR(BatonType, name, returnParameterCount) BatonType() : RTTBaton(returnParameterCount, name) {}


I don't think that this macro serves any purpose, besides it also changes order of arguments.

The only reason this has survived is due to initially wanting to keep it as similar to the highlevel implementation as possible as well as pc-ble-driver-js, but it is redundant here, and I will remove it. Will also consider removing it in highlevel implementation as well.

bencefr · 2017-11-14T13:49:02Z

src/rtt_batons.h

+#include "rtt_helpers.h"
+
+#define RTTBATON_CONSTRUCTOR(BatonType, name, returnParameterCount) BatonType() : RTTBaton(returnParameterCount, name) {}
+#define RTTBATON_DESTRUCTOR(BatonType) ~BatonType()


Will remove.

bencefr · 2017-11-14T13:53:35Z

src/rtt_batons.h

+    bool hasControlBlockLocation;
+    uint32_t controlBlockLocation;
+
+    std::vector<ChannelInfo *> upChannelInfo;


These vectors never seem to be freed, the contents of them I mean. Usage of smart pointers are encouraged.

Will consider it.

bencefr · 2017-11-14T14:03:28Z

src/rtt.cpp

+        while (true) {
+            bool controlBlockFound = false;
+            RETURN_ERROR_ON_FAIL(dll_function.rtt_is_control_block_found(&controlBlockFound), RTTCouldNotCallFunction);
+            std::chrono::high_resolution_clock::time_point attemptedStartupTime = std::chrono::high_resolution_clock::now();


Only used if controlBlockFound is false. Move it after break condition along with a sensible sleep.

bencefr · 2017-11-14T14:08:47Z

src/rtt.cpp

+    {
+        if (baton->returnFunction != nullptr)
+        {
+            std::vector<v8::Local<v8::Value> > vector = baton->returnFunction(baton);


Using variable names matching standard typenames I don't suggest in general, misleading to read.

bencefr · 2017-11-14T14:23:28Z

src/rtt.cpp

+        return RTTSuccess;
+    };
+
+    rtt_return_function_t r = [&] (RTTBaton *b) -> returnType {


returnType might be a magic that the compiler solves here, but does not allow a readable code. Also again vector is a name that I would avoid using.

bencefr · 2017-11-15T07:03:29Z

src/rtt.cpp

+        return baton;
+    };
+
+    rtt_execute_function_t e = [&] (RTTBaton *b) -> RTTErrorcodes_t {


rtt_execute_function_t e = [&] (RTTStartBaton *baton), so you don't need static_cast which should not even exist in C++.

This suggested change results in a compile error, so will keep current implementation.

I see I was wrong, since the function type requires RTTBaton*, however there are better casts probably for this case. Never mind.

bencefr · 2017-11-15T07:04:24Z

src/rtt.cpp

+    return started;
+}
+
+NAN_METHOD(RTT::Start)


Waaaaay too long function, split it up, especially the execute part.

bencefr · 2017-11-15T07:06:01Z

src/rtt.cpp

+        for (auto element : baton->downChannelInfo)
+        {
+            Nan::Set(downChannelInfo, Convert::toJsNumber(i), element->ToJs());
+            i++;


i++ is bad practice, no matter how long you can talk about compiler magic. Get used to use ++i.

bencefr · 2017-11-16T07:30:55Z

In my opinion - in the light of the threading issues - it is a bad approach to lock everything out so brutally. Especially that the library load/unload is also part of this (however it was much much more unsafe before).

The problem is that we are tackling a problem here which is not the context of this feature PR. Here's what has to be done:

finally solve the library loading part, do NOT load and unload for each call.
whatever function is predictably trivial in execution shall be implemented WITHOUT creating a baton and queuing on UV work thread pool, e.g. GetDllVersion().
map all the data that is shared between potential threads and protect the data by locking them individually for the shortest possible access.
consider using a separate UV loop than the default, configure it for your own purposes.

CoqRogue · 2017-11-16T09:30:13Z

In principle, I agree with you @bencefr, there are, however, some other considerations to make.

We have had so many issues with locking the drivers underneath in previous projects. One usecase that often pop up is that people want to do other operations, like programming with the nrfjprog command line tools. And when we load the nrfjprog drivers, they will not be able to perform those operations.
There are a couple of functions, including GetDllVersion, that are fast enough to be executed synchronously without lagging the js-thread. In my opinion, api consistency is more important than advantages to we may get due to moving them to a synchronous api.
I have mapped the data, and we are locking for a minimum time. The speedups we could get by moving the locks is in the a few clock cycles.
I cannot see what kind of separate UV loop that we could setup which would be more practical or useful.

bencefr · 2017-11-16T09:43:33Z

@CoqRogue ,

It is a good thing that you cannot perform anything on an open device while some other application keeps it open. Therefore API has to provide an open/close functionality, which MUST be explicitly called. It is the responsibility of the application using this library to close a device when not used.
API consistency has nothing to do with unnecessarily invoking an other thread.
Loading and unloading a library I do not believe to in the few clock cycle range, especially on Windows.
Separate EV loop was only a suggestion, I am not very familiar with it, but if that would give more control of the contexts it would worth spending a bit of time to investigate. Maybe not.

IvanSanchez · 2017-11-16T09:49:04Z

index.js

@@ -44,3 +44,4 @@
 });

 module.exports = instance;
+ module.exports.RTT = nRFjprog.RTT;


How about changing this to module.exports.RTT = nRFjprog.RTT()?

I'd prefer the module exporting a RTT singleton, instead of a RTT constructor.

Agreed. Will fix.

CoqRogue · 2017-11-16T09:57:47Z

nRFjprog has an optional open/close mechanism which will not load/unload the library. RTT has start which loads the library, and stop that unloads it.
API consistency is one of many factors to consider. In this case I find consistency more important than the minimal gain you could get with doing this synchronously. It is also easier to maintain.
The lock is locked just before the first variable that could come into a race condition and unlocked on any return. The clock cycles I am talking about are the ones at the end of HighLevel::ExecuteFunction: The if (executeError != SUCCESS) {...} could be moved to the bottom of the function and unlock could have been called explicitly right before. We are most likely talking 10-100 clock cycles.
I agree that it could be worth an investigation, but I am not sure that this is the time for it.

bencefr · 2017-11-16T10:02:40Z

the mutex lock is inside the execute function which also locks loadDll() and unloadDll().
I'm not saying it has to be fixed for this PR, this one obviously works. But this has to be addressed later.

bihanssen · 2017-11-17T08:11:37Z

Good job on the code&review all, looks good.

Per Kristian Schanke added 30 commits October 24, 2017 13:48

Load nRFjprog library.

265e0d2

Initial, faultprone, versions of rtt start, stop, and read.

3133f72

WIP: Enable read.

3fcd217

WIP: Add Write and return of raw data on read.

3643d23

WIP: Wrap library names in platform specific filename wrappers.

f821233

Fix some Linux/OS X build issues.

fb5aa9c

Remove unused includes.

a09fac7

Correct include level.

1ae31d9

Fix Linux issues.

642c940

Variable name typo.

b72c515

Remove accidental )

cd7c318

Fix OS X issues.

d9ffbd3

Fix diffs between Linux and OS X

34eeef7

Improve way to find name of library files.

c6cf401

Make it build on *nix

19de27f

Improve errormessages.

11dd0ab

Fix issue where application would not terminate when done.

118d2d7

Remove forgotten use of variable.

4c55a28

Revert "Remove forgotten use of variable."

c5fa7ad

This reverts commit 4c55a28.

Revert "Fix issue where application would not terminate when done."

ada71f3

This reverts commit 118d2d7.

Fix issues with cleaning up resources when starting RTT.

3d1324e

Add tests of RTT.

9ebeb91

Write the correct amount of data.

19760f1

WIP: RTT tests.

fc185e8

Merge remote-tracking branch 'origin/master' into feature/rtt-support

2195abf

# Conflicts: # src/highlevel.cpp # src/highlevel.h # src/highlevel_common.h # src/highlevel_helpers.h # src/platform/linux/libraryloader.cpp # src/platform/osx/libraryloader.cpp

Remove __ from start and end of include guard defines.

ef5996d

WIP: Add timestamps

9acbdf3

Add timestamps to get an idea on when data is produced on the device.

Merge remote-tracking branch 'origin/master' into feature/rtt-support

fb0da1b

Clean up return function.

dc23557

bencefr reviewed Nov 14, 2017

View reviewed changes

IvanSanchez suggested changes Nov 14, 2017

View reviewed changes

bencefr reviewed Nov 14, 2017

View reviewed changes

bencefr reviewed Nov 15, 2017

View reviewed changes

Per Kristian Schanke added 5 commits November 15, 2017 11:02

Update in accordance to review.

0e10e4b

Force C++14. Added forgotten file.

025e978

Update Highlevel with the same fixes that RTT got.

f8b1652

Add a mutex around the code execution to avoid race conditions.

20367b4

Add a mutex around the code execution to avoid race conditions.

bacca7d

IvanSanchez suggested changes Nov 16, 2017

View reviewed changes

Per Kristian Schanke added 2 commits November 16, 2017 11:07

Export an instance of RTT instead of constructor.

d11a232

Confirm that multiple functions can be called in a row.

cb393ec

IvanSanchez approved these changes Nov 16, 2017

View reviewed changes

CoqRogue merged commit f9fd70c into master Nov 16, 2017

CoqRogue deleted the feature/rtt-support branch November 16, 2017 12:38


		enum NrfjprogErrorCodesType {

		Success = 0, // Requested operation (operations) were successfully completed.

		RETURN_ERROR_ON_FAIL(dll_function.rtt_start(), RTTCouldNotStartRTT);


		while (true) {


		RETURN_ERROR_ON_FAIL(dll_function.rtt_read_channel_count(&downChannelNumber, &upChannelNumber), RTTCouldNotGetChannelInformation);

		for(uint32_t i = 0; i < downChannelNumber; ++i)

Feature/rtt support #21

Feature/rtt support #21

Conversation

CoqRogue commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanSanchez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bencefr commented Nov 16, 2017

CoqRogue commented Nov 16, 2017

bencefr commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoqRogue commented Nov 16, 2017

bencefr commented Nov 16, 2017

bihanssen commented Nov 17, 2017