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.

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

Implement Covesa CV Forwarder #48

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

noci0001
Copy link

This pr will add the Covesa CV Forwarder component which implements a forwarder based on Covesa guidelines. For now it applies an algorithm to sift through meaningful signals such as Speed, Latitude, Longitude and Timestamp.

Further changes have been applied to the docker-compose, csv-provider. Launch configurations have been also added to allow fast execution and debug of both forwarders.

A custom mapping of csv-provider files has been also created however the csv file provided by GeoTab will have to be remove so that GeoTab can contribute it themselves.

… positional values and implement CurveLogActor
…Writer, build_snapshot/header_measurement functions inside the covesa-cv-forwarder component
…nted but unavailable due to current incompatible rustc toolchain 1.75.0 vs 1.79.0) and remove docker-compose.yaml
Copy link

@bs-jokri bs-jokri left a comment

Choose a reason for hiding this comment

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

@noci0001 could you please review my comments

.gitignore Outdated
@@ -1,4 +1,4 @@
# SPDX-FileCopyrightText: 2023 Contributors to the Eclipse Foundation
# SPDX-FileCopyrightText: 2025 Contributors to the Eclipse Foundation

Choose a reason for hiding this comment

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

why 2025 and not 2024

Copy link
Author

Choose a reason for hiding this comment

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

adjusted

DEPENDENCIES Outdated

Choose a reason for hiding this comment

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

Where is the DEPENDENCIES file? It is required and needs to contain the result of the latest run of the dash tool

Copy link
Author

Choose a reason for hiding this comment

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

I discarded it because I thought of it as an accidental byproduct of git actions. I reinstated it

@@ -1,5 +1,4 @@

Apache License
Apache License

Choose a reason for hiding this comment

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

How comes that the white space is removed? License files should never change. Possible to revert?

Copy link
Author

Choose a reason for hiding this comment

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

Done

pub time: u128,
}

#[allow(dead_code)]

Choose a reason for hiding this comment

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

Why do we need to allow dead code here? Obviously it is used

Copy link
Author

Choose a reason for hiding this comment

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

I also think it must have been used however I had to add this to satisfy the compiler

lon_datapoints: Vec<Option<f64>>,
time_speed_datapoints: VecDeque<Option<u128>>,
publisher_sender: tokio::sync::mpsc::Sender<Vec<Option<ChosenSignals>>>,
}

Choose a reason for hiding this comment

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

add a blank line

Copy link
Author

Choose a reason for hiding this comment

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

done

vss_data.push(data_entry);
});
}
// loop trough dataentries to extract speed,lat,lon

Choose a reason for hiding this comment

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

data entries

Copy link
Author

Choose a reason for hiding this comment

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

Done

});
}
// loop trough dataentries to extract speed,lat,lon
// reforfm into matching statement

Choose a reason for hiding this comment

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

reforfm= -> reformat?

Copy link
Author

Choose a reason for hiding this comment

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

done

/// # Examples
///
/// ```
/// use clap::Command;

Choose a reason for hiding this comment

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

Why commenting out code snippets. Should that be removed?

Copy link
Author

Choose a reason for hiding this comment

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

done


configs:
influxdb_init.sh:
file: "./influxdb/init-scripts/create-fms-token.sh"

Choose a reason for hiding this comment

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

it feels a bit weird that the covesa-cv-forwarder docker compose file contains that much occurrences of the abbreviation "fms". Possible to rename some of them? (maybe not this one as it is a shared script with the sms-forwarder)

Copy link

Choose a reason for hiding this comment

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

yeah I agree with this one as we need to abstract away from any specific standard

Copy link
Author

Choose a reason for hiding this comment

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

I could rename them from fms to covesa

Choose a reason for hiding this comment

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

what changes happened to this file?

Copy link
Author

Choose a reason for hiding this comment

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

These lines were not supposed to be deleted, I restored them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants