Skip to content
This repository has been archived by the owner on Sep 2, 2024. It is now read-only.

initial CSV-recorder #105

Merged

Conversation

eriksven
Copy link
Contributor

Adds a script for recording files according to the format expected by the CSV-provider

csv_provider/README.md Outdated Show resolved Hide resolved
csv_provider/README.md Outdated Show resolved Hide resolved
| short argument | long argument | description | default value |
|---- | ---- | ----- | ----|
|-f| --file | This indicates the filename to which to write the VSS-signals. | signalsOut.csv |
|-s| --signals | A list of signals to record. | signalsOut.csv | |
Copy link
Contributor

Choose a reason for hiding this comment

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

Default is incorrect? Shall we mention how you specify multiple signals, i.e. one -s with multiple names separated by comma?

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 now extend the example call above to show how one can specify multiple signals even without the comma like in -s Vehicle.Speed Vehicle.Width. WDYT?

Copy link
Contributor

@lukasmittag lukasmittag left a comment

Choose a reason for hiding this comment

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

And just one quick comment. If you set the same value multiple times it will only record only once because databroker just acts on it once. Just letting you know :)

|-s| --signals | A list of signals to record. | There is no default value, but the argument is required.| |
| -a | --address | This indicates the address of `kuksa.val` databroker to connect to. | 127.0.0.1 |
| -p | --port | This indicates the port of the `kuksa.val` databroker to connect to. | 55555 |
| -l | --log | This sets the logging level. Possible values are: DEBUG, INFO, DEBUG, WARNING, ERROR, CRITICAL | INFO
Copy link
Contributor

Choose a reason for hiding this comment

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

two times DEBUG

Copy link
Contributor

Choose a reason for hiding this comment

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

@lukasmittag
Copy link
Contributor

also one conflicting file I see

def init_argparse() -> argparse.ArgumentParser:
'''This inits the argument parser for the CSV-recorder.'''
parser = argparse.ArgumentParser(
usage="-a [BROKER ADDRESS] -p [BROKER PORT] -f [FILE]",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should -s and -l be listed on this line as well?

'delay': time_gap})
except VSSClientError as error:
logging.error("There was a problem in the interaction"
" with the Kuksa.val data broker at %s:%s: %s ",
Copy link
Contributor

Choose a reason for hiding this comment

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

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 will replace it to align, but personally I prefer 'data broker' since most language checkers ask for this writing and I do not think the term is unique enough to be a proper noun in the context of Eclipse Kuksa.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no strong opinion on what we use as long as we are consistent. At the moment we mostly use "Databroker", in for example the README, but if we would agreed that "Data broker" would be better name I have no problem with that, but then we should change it "Data broker" in all free-text-places, and possibly use "data-broker" or "data_broker" as preferred name where blanks cannot be used, like directories and docker containers.

@erikbosch erikbosch changed the title initial CSV-recoder initial CSV-recorder Jul 6, 2023
@@ -7,7 +7,7 @@ The provider requires an installation of Python in version 3 and can be executed

```
pip install -r requirements.txt
python provider.py
python3 provider.py
Copy link
Contributor

Choose a reason for hiding this comment

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

@SebastianSchildt - I have noticed that we are quite inconsistent regarding if we use python or python3 in our examples. We definitively cannot use "python2" but in most distros "python2" isn't available anyway, and it might be so that your python command anyway points to a python3 installation. We also likely do not support older 3.X-versions anyway.

But in short - do we prefer to use python or python3 in examples? If we agree on preferred term I have an idea of aligning the examples so that they all use the same term. A similar discussion exists for pip vs pip3.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I prefer python3 because I use it my own but no „strong“ opinion

Copy link
Contributor

Choose a reason for hiding this comment

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

Just saw the MS Teams bot ate my answer, I wrote this

No strong opinion either. Asking me like this I'd prefer just "python", I do have a vague memory that some of the python3 where needed in the past, as the default was Py2 on some distros also used in containers. But that may have been ancient Ubuntu 16.04 times, so I'd be fine to use "pip" and "python", if container builds break we will see it. From user perspective, depending what environment they use, they can fall into a hole either way....

@SebastianSchildt
Copy link
Contributor

lgtm.

@SebastianSchildt SebastianSchildt merged commit 3a72e0b into eclipse-kuksa:main Aug 21, 2023
5 checks passed
@erikbosch erikbosch deleted the feature/csv-recorder branch October 31, 2024 13:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants