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.

Sign up for GitHub

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

Jump to bottom

Update ganglion decompression #655

Merged
merged 5 commits into from
Aug 4, 2023
Merged

Update ganglion decompression #655

merged 5 commits into from
Aug 4, 2023

Conversation

philippitts
Copy link
Contributor

No description provided.

@philippitts philippitts force-pushed the update-ganglion-decompression branch from bd43203 to 3293a2c Compare July 28, 2023 23:16
@philippitts philippitts force-pushed the update-ganglion-decompression branch from 3293a2c to 4579b82 Compare August 1, 2023 21:10
@philippitts philippitts marked this pull request as ready for review August 1, 2023 21:11
Andrey1994
Andrey1994 reviewed Aug 1, 2023
View reviewed changes
Copy link
Member

@Andrey1994 Andrey1994 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

looks ok, I wrote some comments but mostly because its alway scary to touch some code for ganglion..
there is a set of tests we need to pass before merging it:

  • old compression works for raw data for both boards
  • new compression works for raw data for both boards
  • impedance checking works for old and new compression for board boards
  • text messages sent from ganglion(bytes 200-207) work for both boards and both algorithms

src/board_controller/openbci/ganglion.cpp Outdated
@@ -479,6 +396,138 @@ void Ganglion::read_thread ()
delete[] package;
}

void Ganglion::decompress_firmware_3 (

Copy link
Member

Choose a reason for hiding this comment

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

strange that clang format does not remote it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to make it one line, but the clang-format settings move the parameters to the second line. I think it may be because the line is longer than the max line length. Is there a different format you'd like this in?

Copy link
Member

Choose a reason for hiding this comment

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

I mean empty line between function name and arguments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh weird - should be fixed now.

src/board_controller/openbci/ganglion.cpp Outdated
@@ -592,7 +641,22 @@ int Ganglion::call_open ()
}

safe_logger (spdlog::level::info, "search for {}", params.mac_address.c_str ());
res = func (const_cast<char *> (params.mac_address.c_str ()));

struct ConnectionParameters
Copy link
Member

Choose a reason for hiding this comment

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

I dont like strcut definition like this in cpp code and it seems like its duplicated between ganglion.cpp and bglib, can be moved to a common header I think. Also, compiler may align it randomly somewhere(its unlikely)

src/board_controller/openbci/ganglion_bglib/callbacks.cpp Outdated
// Determine the firmware version from the advertised name
if (msg->data.data[0] == 20 && msg->data.data[13] == '3')
{
GanglionLib::firmware = 3;
Copy link
Member

Choose a reason for hiding this comment

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

I think this entire if block should be under the check for expected name pattern

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the note about the serial_number parameter below. If a specific device name is passed to brainflow, the firmware must still be determined. Putting this block outside of the name and serial_number checks prevents duplicating the same logic in both locations.

Copy link
Member

Choose a reason for hiding this comment

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

I mean that its probably more reliable to check fw after validating the name, some data from other devices can be send and first they should be filtered by the name. Here you will set fw value wo such filtering

Copy link
Member

Choose a reason for hiding this comment

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

or two ganglions in the same place with two different fw versions, wo check for name you can mess up here

src/board_controller/openbci/ganglion_bglib/callbacks.cpp Show resolved Hide resolved
src/board_controller/openbci/ganglion_bglib/main.cpp Outdated
@@ -100,7 +107,15 @@ namespace GanglionLib
}
exit_code = (int)CustomExitCodes::SYNC_ERROR;
state = State::OPEN_CALLED;
char *mac_addr = (char *)param;

struct ConnectionParameters
Copy link
Member

Choose a reason for hiding this comment

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

common definition in header will be better

src/board_controller/openbci/ganglion_native.cpp Show resolved Hide resolved
@philippitts philippitts force-pushed the update-ganglion-decompression branch from 016c66c to 899f967 Compare August 3, 2023 19:20
@philippitts
Copy link
Contributor Author

looks ok, I wrote some comments but mostly because its alway scary to touch some code for ganglion.. there is a set of tests we need to pass before merging it:

* old compression works for raw data for both boards

* new compression works for raw data for both boards

* impedance checking works for old and new compression for board boards

* text messages sent from ganglion(bytes 200-207) work for both boards and both algorithms

Test results below. Old compression isn't supported on a device with the new firmware so I don't think we can test both compression types with both boards, but I will show examples of both the new and old firmwares working. Let me know if you had something else in mind.

I'm also not sure what makes sense to test for the text messages sent from Ganglion. The impedance values are sent in bytes 201 - 205. You can query the device registers and get text message responses, but AFAIK there is no way in brainflow to send this command.

This is a 4mV at 4Hz sine wave using a board with firmware 3.0.0 (new)
image (1)

This is a 4mV at 4Hz sine wave using a board with firmware 2.0.1 (old). It looks bad, but this is expected because this was one of the cases where the old compression algorithm exhibited failures. It is similar to results for the same sine wave using the latest brainflow release.
image (2)

This is the impedance check running with the new firmware. I don't have leads on me right now, but the channels do generally respond as expected when I touch the pins. There are no crashes or errors, and the responses are similar on both the old and new firmwares. We will do more comprehensive testing (with leads) after assembling the full tech stack.
image

This is the impedance check running with the old firmware.
image

@Andrey1994 Andrey1994 merged commit 0dbd69d into brainflow-dev:master Aug 4, 2023
11 checks passed
@Andrey1994
Copy link
Member

looks ok, I will review and merge PR for freeeg128 and create a new release this weekend

@philippitts philippitts deleted the update-ganglion-decompression branch August 7, 2023 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Andrey1994 Andrey1994 Andrey1994 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@philippitts @Andrey1994