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

Feature/structure engine fluids #710

Merged
merged 8 commits into from
Mar 11, 2024

Conversation

kowend
Copy link
Contributor

@kowend kowend commented Jan 30, 2024

Several engine oil and coolant related attributes where spread in the main branch of the combustion engine.
The engine oil and coolant have been put under a branch.
Additional coolant lifespan and level (ISO 7000 2429) have been added under the coolant branch.

@kowend kowend force-pushed the feature/structureEngineFluids branch from 99d5cce to 5a2165f Compare January 30, 2024 15:31
@erikbosch erikbosch added Scope:Major A change that either significantly adds new functionality, or affects backward compatibility. Status:New An issue/PR that not yet have been discussed/announced at a VSS meeting labels Jan 31, 2024
@kowend kowend force-pushed the feature/structureEngineFluids branch from f505f2c to aadf20b Compare January 31, 2024 09:59
@erikbosch
Copy link
Collaborator

Topics to discuss:

  • This is right now considered as a "major" change (to be included in a major release only) as some signals will change name
  • Alternatively, one could for now "keep" the old names (in addition to the new ones) but mark them as deprecated

@kowend
Copy link
Contributor Author

kowend commented Feb 1, 2024

I think marking them as deprecated is fine, and leave them in. Is there any standard/rule for removing deprecations?
I can imagine we don't want too many deprecations left in the specs.

For which version would these signals then be deprecated?

@kowend
Copy link
Contributor Author

kowend commented Feb 1, 2024

Ah nevermind about the rule for deprecation. I found the description in the basics.md. But knowing the version for deprecation would still be useful. Would that be v5.0?

Signed-off-by: Koen Deleij <[email protected]>
…he reference to the EV engine to keep the coolant consistent across powertrain types

Signed-off-by: Koen Deleij <[email protected]>
@erikbosch
Copy link
Collaborator

Ah nevermind about the rule for deprecation. I found the description in the basics.md. But knowing the version for deprecation would still be useful. Would that be v5.0?

Possibly. Historically we have selected "next release" as "deprecated from", but right now we have not really decided if we will release a 4.2 or if next release will be 5.0. I would say that 5.0 is the best option for now for main, if we decide to create a 4.2 then we as maintainers should anyway check all deprecation comments and adjust version if needed, like replacing 5.0 with 4.2 for the maintenance branch.

@kowend
Copy link
Contributor Author

kowend commented Feb 1, 2024

Clear! I'll add the deprecation attributes.
I've realised that the electrical motor could also have a cooling compartiment. At least the coolant temperature is mentioned as an optional.
I've moved the coolant branch into a separate spec file.

Copy link
Collaborator

@ppb2020 ppb2020 left a comment

Choose a reason for hiding this comment

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

A few minor comments.

@@ -0,0 +1,38 @@
# Copyright (c) 2016 Contributors to COVESA
Copy link
Collaborator

Choose a reason for hiding this comment

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

2016 -> 2024?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In one way 2016 can make sense, as some of the signals actually have been copied from another file and maybe they were added in 2016, I have not checked. So even if the file is created 2024, the content is older

allowed: [
'CRITICALLY_LOW', # Critically low, immediate action required
'LOW', # Level below recommended range, but not critical
'NORMAL', # Within normal range, no need for driver action
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this also have a HIGH and CRITICALLY_HIGH level as for EngineOil.Level?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would at least not hurt. I actually do not know what sensors modern high-end vehicles have for coolant; like can they detect and report that there is too much coolant?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know enough about the mechanisms, but one possibility may be that the same kind of sensor could be used for both, for example. As well, what I am proposing seems to provide consistency that could be reused for other "level" sensors (washer fluid, transmission fluid, etc.). We do have another issue tackling common signals. Maybe we should consider a fluid-level signal that can be re-used for all these kinds of sensors?

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 did consider reusing this and have the same enums. Technically I don't know what a typical sensor for the coolant could do; From a functional perspective I don't know if it makes sense to have HIGH and CRITICALLY_HIGH. For the engine oil I can imagine that having the right amount between 2 thresholds is important. But with coolant I don't think there is an upper threshold (other than you'll probably get wet shoes).
ISO 7000 2429, which is meant for the coolant level telltale describes 'low' or 'out of specified values'. In that case it would maybe make more sense to have something like 'NORMAL', 'LOW', 'CRITICAL'.

@erikbosch
Copy link
Collaborator

MoM:

  • Please review

@erikbosch erikbosch added Scope:Minor A change that is not major and not trivial. Status:Meeting Intended to be discussed at next VSS-project meeting and removed Status:New An issue/PR that not yet have been discussed/announced at a VSS meeting Scope:Major A change that either significantly adds new functionality, or affects backward compatibility. labels Feb 13, 2024
@erikbosch
Copy link
Collaborator

MoM:

  • Pierre Pierre not present
  • Merge decision next week unless comments/remarks

@erikbosch
Copy link
Collaborator

MoM:

  • Pierre on vacation, keep open. He is back next week

@erikbosch
Copy link
Collaborator

MoM:

  • Possible merge decision next week
  • @ppb2020 could you check if it is ok

@ppb2020
Copy link
Collaborator

ppb2020 commented Mar 5, 2024

I have no further comments. I am fine with the responses that were provided, with a note that I am still slightly in favour of reusing the set of enum values, but not enough to block merge.

@erikbosch
Copy link
Collaborator

MoM:

  • Merge

@erikbosch erikbosch merged commit f3b7c0f into COVESA:master Mar 11, 2024
4 checks passed
erikbosch pushed a commit to boschglobal/vehicle_signal_specification that referenced this pull request May 6, 2024
* Restructured the coolant end engine oil into 2 branches

Signed-off-by: Koen Deleij <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope:Minor A change that is not major and not trivial. Status:Meeting Intended to be discussed at next VSS-project meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants