Add an optional LibFTDI serial backend #218

xloem · 2021-01-28T20:04:58Z

This adds an alternative serial backend using libftdi. The backend is enabled by the cmake flag USE_LIBFTDIPP. libftdi itself needs to be built with its FTDIPP flag, to enable C++ bindings. It would be a simple change to the code to use the C library without the C++ bindings, if that's valued (EDIT: this has been done, and configuration flags to libftdi are no longer required. The new cmake flag is just USE_LIBFTDI).

libftdi provides an alternative way to reference device paths. Instead of specifying the serial port as /dev/ttyUSB0, you can for example specify i:0x0403:0x6015:0 which will select the first openbci dongle on the usb bus, or s:0x0403:0x6015:DM03H8Q9 which will select the openbci dongle with the serial number "DM03H8Q9".

My goal in submitting this PR is to just open possible avenues towards implementing serial access on android (#216). Libftdi uses libusb under the hood, and libusb has some support for android already.

I understand changing the backends is not the preferred approach, but I am sharing this in case it is of interest. @Andrey1994 what are your thoughts?

Andrey1994 · 2021-01-29T16:58:23Z

I like it, it looks great!

I will add a test for it to CI pipelines and check that it works the same way

Andrey1994 · 2021-01-30T23:56:27Z

src/utils/serial.cpp

+                log_error ("read_from_serial_port");
+                return bytes_read;
+            }
+            next_bytes += bytes_read;


why do you need to check in a loop manually here? looks like there is timeout configured(ctx.set_usb_read_timeout (ms_timeout);) it should handle this case I guess

And I dont fully understand this statement in while loop next_bytes == bytes_to_read

thanks for looking at these things !

Unfortunately it respects the timeout only for the usb bus. The ftdi chip will actually send an empty data packet after its configured "latency" delay expires, which the library treats as a response. See http://www.ftdichip.com/Support/Documents/AppNotes/AN232B-04_DataLatencyFlow.pdf page 8 item 4; there's also more information elsewhere in that document. Even if the usb timeout is set to 1 second, the chip will usually reply with its empty data buffer after only 16 ms.

next_bytes == bytes_to_read is true when no data has been received yet. I can probably add a flag to make that clearer.

"next_bytes == bytes_to_read is true when no data has been received yet. I can probably add a flag to make that clearer"
yep, I realise it. But should there be a check that size > 0 in this loop?

I pushed another commit where the loop is recoded to clarify that it stops as soon as size > 0 . I think I hadn't made up my mind whether to do that, when I first coded it.

oh! yes there is an edge case if 0 is passed to the function. i'll try to address that in a second push.

I mean that in a loop you read bytes and decrease size variable by bytes read so loop should be stopped when its 0

at that point next_bytes == bytes_to_read would have been false, stopping the loop (edit: unless size was passed as 0, which was the edge case). is the new loop structure clearer? it's intended to only performs 1 read that returns data. it could loop until size is read, doesn't seem to really matter.

Andrey1994 · 2021-01-30T23:59:15Z

src/utils/serial.cpp

+
+    void log_error (const char *action)
+    {
+        logger->error ("libftdi {}: {} -> {}", description, action, ctx.error_string ());


lets rm this logger completely or replace it by board_logger, this way logging behavior will be under user's control from high level api

I was actually trying to connect to board_logger here, see this line: https://github.com/brainflow-dev/brainflow/pull/218/files#diff-707d50ff0c1f53ceed4b00bbb44de221f4e4804adeb6c8a62059677a2f153933R39 . What is the right way to connect to board_logger ? Maybe add a constructor parameter and pass it in?

Its a public static field in a board class, I guess you can use forward declaration or include header(maybe it will lead to circular includes).

Its preferred to use safe_logger instead spdlog directly, its a non static method of board class. It should be possible to pass it as an argument in constructor

src/utils/serial.cpp

Andrey1994 · 2021-01-31T01:32:34Z

would be great to add a CI test for it, it will also help to understand which packages should be preinstalled in order to use this lib

xloem · 2021-01-31T01:51:48Z

TODO for this PR:

change logger to use board logger ( Add an optional LibFTDI serial backend #218 (comment) ), or remove it
add CI test that demonstrates installation of libftdi

Andrey1994 · 2021-01-31T20:35:11Z

https://github.com/brainflow-dev/brainflow/blob/master/.github/workflows/run_libftdi.yml#L35 lets change this to add libftdi checks to ci pipeline

This lets it work out of the box on more setups.

Alternatives to this include moving safe_logger out of the board class, or throwing exceptions for errors.

xloem · 2021-02-02T22:42:36Z

@Andrey1994 I think I've implemented the two items.

Here are some further thoughts:

It might be organised to move each serial implementation into its own file and class or somesuch. I didn't do this only to make there be fewer changes to review. Notably, there's no need to expose OSSerial in the main header file, and it's not balanced with LibFTDISerial being unexposed. It could also be hard and confusing for future developers to navigate serial.cpp when it's so long and has function implementations with duplicate names.
i'm used to managing errors with exceptions rather than return codes in C++. not sure what the rest of the library is using, but i copied the other serial classes and used return codes.
i noticed board_logger is statically defined, so it's important never to use it in something called from another static definition, or it could segfault if they are linked in the wrong order. a fix to this is to change it to a static function that returns a static local variable, which will then be constructed on first call.
i don't see the purpose of skip_logs yet, given that board_logger is statically defined and should outlive destructors. I built brainflow with the safe_logger check disabled and ran into no associated issues in get_data_demo or CI. i'm inferring it's something inside spdlog?

Andrey1994 · 2021-02-03T23:09:14Z

"i don't see the purpose of skip_logs yet, given that board_logger is statically defined and should outlive destructors. I built brainflow with the safe_logger check disabled and ran into no associated issues in get_data_demo or CI. i'm inferring it's something inside spdlog?"

loggers used inside release_session method and its fine. Destructor calls release_session method and its needed to handle the case if user didnt call release_session manually.

spdlog has a known issue that it should not be called from destructors when program ends, some objects in spdlog released automatically before and it leads to a crash

Andrey1994 · 2021-02-03T23:12:43Z

"i'm used to managing errors with exceptions rather than return codes in C++. not sure what the rest of the library is using, but i copied the other serial classes and used return codes."

here exceptions are not used, this code is called from other languages and they can get return code from C\C++ but they can not handle exception in native code

Andrey1994 · 2021-02-08T03:47:00Z

#221

feel free to review this PR, I splitted it by different files

* Update settings.xml * Refactor brainflow_boards and board classes and implement marker api (brainflow-dev#212) * add insert_marker method Signed-off-by: Andrey Parfenov <a1994ndrey@gmail.com> * bump julia version Signed-off-by: Andrey Parfenov <a1994ndrey@gmail.com> * docs update Signed-off-by: Andrey Parfenov <a1994ndrey@gmail.com> * CI for libFTDI (brainflow-dev#219) * add template for libftdi ci job Signed-off-by: Andrey Parfenov <a1994ndrey@gmail.com> * Update run_libftdi.yml * hotfix for markers on oymotion Signed-off-by: Andrey Parfenov <a1994ndrey@gmail.com> * bump julia version Signed-off-by: Andrey Parfenov <a1994ndrey@gmail.com> * disable fascia tests temporary Signed-off-by: Andrey Parfenov <a1994ndrey@gmail.com> * adding opencollective to sponsor button Signed-off-by: Andrey Parfenov <a1994ndrey@gmail.com> * Add an optional LibFTDI serial backend (brainflow-dev#218) * Add an optional LibFTDI1++ serial backend * fixing marker type (brainflow-dev#220) * fix marker type Signed-off-by: Andrey Parfenov <a1994ndrey@gmail.com> * bump julia version Signed-off-by: Andrey Parfenov <a1994ndrey@gmail.com> * refactor osserial and libftdiserial (brainflow-dev#221) * refactor osserial and libftdiserial Signed-off-by: Andrey Parfenov <a1994ndrey@gmail.com> * hacked libftdi into android ci Co-authored-by: Andrey Parfenov <a1994ndrey@gmail.com>

xloem · 2021-02-19T12:09:40Z

spdlog has a known issue that it should not be called from destructors when program ends, some objects in spdlog released automatically before and it leads to a crash

Andrey1994 do you have any link to this issue? I didn't manage to find it at the spdlog repository. It could be the same issue of static initialisation/destruction order.

EDIT: Andrey answered in discord. Two links were gabime/spdlog#1738 and
gabime/spdlog#1050 (comment)

Andrey1994 reviewed Jan 31, 2021

View reviewed changes

Andrey1994 mentioned this pull request Jan 31, 2021

CI for libFTDI #219

Merged

xloem added 7 commits February 2, 2021 15:53

Add an optional LibFTDI1++ serial backend

a295a17

doc fix

53c72c9

clarify read timeout loop

0b5d9c3

catch edge case where size<=0 is passed to read func

cc631d0

disable tcflush for older libftdi versions without it

cd438d9

downported libftdi++ code to plain c libftdi

7550b63

This lets it work out of the box on more setups.

formatted serial.cpp

62df083

xloem force-pushed the libftdi branch from e114cbc to 62df083 Compare February 2, 2021 20:54

xloem added 3 commits February 2, 2021 16:09

libftdi enabled for ci

18ffa99

Change libftdi logging to use a board object.

dd38e81

Alternatives to this include moving safe_logger out of the board class, or throwing exceptions for errors.

clang-format src/utils/inc/serial.h

b73063e

downgrade libftdi1pp to libftdi on ubuntu workflow

eac8be2

Andrey1994 merged commit 366ad0d into brainflow-dev:master Feb 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an optional LibFTDI serial backend #218

Add an optional LibFTDI serial backend #218

xloem commented Jan 28, 2021 •

edited

Loading

Andrey1994 commented Jan 29, 2021

Andrey1994 Jan 30, 2021 •

edited

Loading

Andrey1994 Jan 31, 2021

xloem Jan 31, 2021 •

edited

Loading

Andrey1994 Jan 31, 2021

xloem Jan 31, 2021

xloem Jan 31, 2021

Andrey1994 Jan 31, 2021

xloem Jan 31, 2021 •

edited

Loading

Andrey1994 Jan 30, 2021

xloem Jan 31, 2021 •

edited

Loading

Andrey1994 Jan 31, 2021

Andrey1994 commented Jan 31, 2021

xloem commented Jan 31, 2021 •

edited

Loading

Andrey1994 commented Jan 31, 2021

xloem commented Feb 2, 2021

Andrey1994 commented Feb 3, 2021

Andrey1994 commented Feb 3, 2021

Andrey1994 commented Feb 8, 2021

xloem commented Feb 19, 2021 •

edited

Loading

Add an optional LibFTDI serial backend #218

Add an optional LibFTDI serial backend #218

Conversation

xloem commented Jan 28, 2021 • edited Loading

Andrey1994 commented Jan 29, 2021

Andrey1994 Jan 30, 2021 • edited Loading

Choose a reason for hiding this comment

Andrey1994 Jan 31, 2021

Choose a reason for hiding this comment

xloem Jan 31, 2021 • edited Loading

Choose a reason for hiding this comment

Andrey1994 Jan 31, 2021

Choose a reason for hiding this comment

xloem Jan 31, 2021

Choose a reason for hiding this comment

xloem Jan 31, 2021

Choose a reason for hiding this comment

Andrey1994 Jan 31, 2021

Choose a reason for hiding this comment

xloem Jan 31, 2021 • edited Loading

Choose a reason for hiding this comment

Andrey1994 Jan 30, 2021

Choose a reason for hiding this comment

xloem Jan 31, 2021 • edited Loading

Choose a reason for hiding this comment

Andrey1994 Jan 31, 2021

Choose a reason for hiding this comment

Andrey1994 commented Jan 31, 2021

xloem commented Jan 31, 2021 • edited Loading

Andrey1994 commented Jan 31, 2021

xloem commented Feb 2, 2021

Andrey1994 commented Feb 3, 2021

Andrey1994 commented Feb 3, 2021

Andrey1994 commented Feb 8, 2021

xloem commented Feb 19, 2021 • edited Loading

xloem commented Jan 28, 2021 •

edited

Loading

Andrey1994 Jan 30, 2021 •

edited

Loading

xloem Jan 31, 2021 •

edited

Loading

xloem Jan 31, 2021 •

edited

Loading

xloem Jan 31, 2021 •

edited

Loading

xloem commented Jan 31, 2021 •

edited

Loading

xloem commented Feb 19, 2021 •

edited

Loading