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

Fix errors in examples #141

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

erikbosch
Copy link
Contributor

@erikbosch erikbosch commented Jul 15, 2024

I performed the following test:

What I fixed:

  • Complaints on velocitas init (except VSS version missmatch)
  • Python errors when trying to start app (typicall sdv_modelnot found)

What I did not fix:

  • App business logic inconsistencies and runtime errors, like that some examples uses VSS 4.1 but tooling use unit file from 4.0?

@erikbosch erikbosch force-pushed the erik_doc branch 2 times, most recently from 9e00e19 to abdb09a Compare July 17, 2024 10:47
@erikbosch erikbosch requested a review from MP91 July 17, 2024 10:53
@erikbosch erikbosch changed the title Fix proto link for dog mode Fix errors in examples Jul 17, 2024
Copy link
Contributor

@MP91 MP91 left a comment

Choose a reason for hiding this comment

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

IMO if we now do some fixes on the examples we should align everything, that means we should use VSS 4.0 everywhere and also adapt the datapoints accordingly.

Furthermore we should align the structure to have a main and vapp file as in the seat-adjuster example.

If we don't do it correct IMO this PR is not helping a lot, since we still need additional work...

Comment on lines 32 to 37
"type": "grpc-interface",
"config": {
"src": "https://github.com/eclipse/kuksa.val.services/blob/v0.2.0/hvac_service/proto/sdv/edge/comfort/hvac/v1/hvac.proto",
"src": "https://raw.githubusercontent.com/eclipse/kuksa.val.services/v0.2.0/hvac_service/proto/sdv/edge/comfort/hvac/v1/hvac.proto",
"required": {
"methods": [
"SetAcStatus",
Copy link
Contributor

Choose a reason for hiding this comment

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

should we remove that completely, since we have the service already defined in the custom model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to make sense - we could actually also remove the VSS reference, it isn't used

examples/dog-mode/AppManifest.json Show resolved Hide resolved
@erikbosch
Copy link
Contributor Author

IMO if we now do some fixes on the examples we should align everything, that means we should use VSS 4.0 everywhere and also adapt the datapoints accordingly.

Furthermore we should align the structure to have a main and vapp file as in the seat-adjuster example.

If we don't do it correct IMO this PR is not helping a lot, since we still need additional work...

How is it as of today? Is Databroker always started with default VSS catalog when starting runtime, i.e. not considering what is written in appManifest? If so it makes sense to adapt examples so they all use default VSS version. I did this work mostly to validate that there are no regressions coming from #139 which does not seem to be the case, if we are to verify/update all aspects of each example more time would be needed.

It also appears to me that many examples are quite similar, so do we need them all? If we would pend more time on them it would also be great if they could be tested as part of CI when updating SDK. That does not happen today, right?

@erikbosch erikbosch mentioned this pull request Jul 17, 2024
1 task
@MP91
Copy link
Contributor

MP91 commented Jul 17, 2024

IMO if we now do some fixes on the examples we should align everything, that means we should use VSS 4.0 everywhere and also adapt the datapoints accordingly.
Furthermore we should align the structure to have a main and vapp file as in the seat-adjuster example.
If we don't do it correct IMO this PR is not helping a lot, since we still need additional work...

How is it as of today? Is Databroker always started with default VSS catalog when starting runtime, i.e. not considering what is written in appManifest? If so it makes sense to adapt examples so they all use default VSS version. I did this work mostly to validate that there are no regressions coming from #139 which does not seem to be the case, if we are to verify/update all aspects of each example more time would be needed.

It also appears to me that many examples are quite similar, so do we need them all? If we would pend more time on them it would also be great if they could be tested as part of CI when updating SDK. That does not happen today, right?

According to databroker: velocitas init should download the specified file and add the config for the databroker. so databroker should always be started with the vss file which is defined in the app manifest.

Agreed that reworking the examples can take a lot of time, that's why it did not happen so far :D

Is there maybe an easy solution to at least be able to use the examples in the template without errors? your initial message at least mentions some more errors besides the ones fixed?

@erikbosch erikbosch marked this pull request as draft July 17, 2024 13:41
@erikbosch
Copy link
Contributor Author

Putting this one on hold for now - before merging we should better run through the examples thoroughly and update documentation. Possibly adapt examples so they can be imported by the template.

@BjoernAtBosch
Copy link
Member

I think, it would be already a significant improvement, if we could get rid of using the outdated vehicle-model-python repository with adapting the examples

Also removing links to the obsolete vehicle-model-python repository

Fixes things that seems to be obviously wrong,
resulting in that "velocitas init" fail or that
Removing dead config
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