Feat: create lsl reader and config parser#30
Conversation
MichalSzandar
changed the title
| std::cerr << "LSLReader: fatal error, stopping acquisition: " << e.what() << "\n"; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Per README.md:187-191 should catch lsl::lost_error to resolve a dropped stream. No resolving logic implemented
| GroundConfig ground; | ||
| ImpedanceConfig impedance; | ||
| // TODO: DataWriterConfig writer; // EEG output file format strategy | ||
| }; |
There was a problem hiding this comment.
JSON config representation requires the channels to be a top-level field, while in the CPP struct the field is hidden under lsl.channels. This is a mismatch as everything else is parsed 1:1 and may cause confusion later. I suggest moving the JSON channels to be inside lsl_stream.
|
|
||
| try { | ||
| ExperimentConfig config; | ||
| config.configVersion = requireField<std::string>(root, "config_version", "config root"); |
There was a problem hiding this comment.
config version is never checked.
|
The config boundary between what stays in protobuf files and what goes in via JSON is never documented. This should be established early to avoid later config refactoring. |
|
|
||
| static ExperimentConfig parse(const std::string& filePath); | ||
| static ExperimentConfig parseStream(std::istream& stream); | ||
| }; |
There was a problem hiding this comment.
All-static classes should be replaced by namespaced free functions, e.g.
namespace config {
ExperimentConfig parse(const std::string& path);
ExperimentConfig parseStream(std::istream& stream);
}
This is also a problem with the already established Parser
There was a problem hiding this comment.
No proper validation functions before parsing for every config type. Consider making private constructors with a factory or at least a validate function that can be used before parsing.
This is not a problem if the config's purpose is and will only ever be to pass them into a parser. If so, document it.
There was a problem hiding this comment.
The reader doesn't use the most resource heavy part of LSLConfig, being the channels. Is the LSLConfig only a Data Transfer Object, and if so, should it be used here?
| double thresholdKohm = 0.0; | ||
| }; | ||
|
|
||
| struct ExperimentConfig { |
There was a problem hiding this comment.
Rename ExperimentConfig to e.g. DeviceConfig, HardwareConfig? This has nothing to do with the experiment configuration, which will be stored in the Protobuf files.
M1KUS3Q
left a comment
M1KUS3Q
left a comment
There was a problem hiding this comment.
Left stuff to think about in comments, otherwise lgtm
2 participants
Here is an example of config.json file
{ "config_version": "1.0", "device_name": "OpenBCI Cyton 8ch", "montage_standard": "10-20", "lsl_stream": { "name": "obci_eeg1", "type": "EEG", "source_id": "cyton-a1b2c3", "expected_channel_count": 8, "expected_sample_rate_hz": 250 }, "reference": { "label": "linked_mastoids", "scheme": "physical" }, "ground": { "label": "Fpz" }, "channels": [ { "index": 0, "label": "Fz", "enabled": true, "unit": "microvolts" }, { "index": 1, "label": "Cz", "enabled": true, "unit": "microvolts" }, { "index": 2, "label": "Pz", "enabled": true, "unit": "microvolts" }, { "index": 3, "label": "Oz", "enabled": true, "unit": "microvolts" }, { "index": 4, "label": "P3", "enabled": true, "unit": "microvolts" }, { "index": 5, "label": "P4", "enabled": true, "unit": "microvolts" }, { "index": 6, "label": "O1", "enabled": true, "unit": "microvolts" }, { "index": 7, "label": "O2", "enabled": false, "unit": "microvolts" } ], "impedance_check": { "supported": true, "threshold_kohm": 5.0 } }