-
Notifications
You must be signed in to change notification settings - Fork 1
Incorporate Findings from Review #34
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
base: smt-xg/BatteryPass_Working
Are you sure you want to change the base?
Incorporate Findings from Review #34
Conversation
remove wrong preferred name fix language for preferred name
- fix language issues - formatting
- adding example value for Logo - formatting
add exampleValue for company logo
io.admin-shell.idta.batterypass.carbon_footprint/1.0.0/gen/CarbonFootprintBattery.json
Outdated
Show resolved
Hide resolved
io.admin-shell.idta.batterypass.technical_data/1.0.0/gen/TechnicalDataBattery.json
Outdated
Show resolved
Hide resolved
io.admin-shell.idta.carbon_footprint.pact/1.0.0/gen/CarbonFootprintPact.json
Show resolved
Hide resolved
io.admin-shell.idta.carbon_footprint.pact/1.0.0/gen/CarbonFootprintPact.json
Show resolved
Hide resolved
io.admin-shell.idta.carbon_footprint.pact/1.0.0/gen/CarbonFootprintPact.json
Show resolved
Hide resolved
minor fixes
…for type of email etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still plenty non-printable characters, for example in TechnicalDataBattery.aas.json in many description texts. I think this should be changed.
io.admin-shell.idta.batterypass.technical_data/1.0.0/gen/TechnicalDataBattery.json
Show resolved
Hide resolved
[ samm:property :initialInternalResistanceOfBatteryCell; samm:payloadName "InitialInternalResistanceOfBatteryCell" ] | ||
[ samm:property :initialInternalResistanceOfBatteryPack; samm:payloadName "InitialInternalResistanceOfBatteryPack" ] | ||
[ samm:property :initialInternalResistanceOfBatteryModule; samm:optional true; samm:payloadName "InitialInternalResistanceOnBatteryModule" ] | ||
[ samm:property :internalResistanceIncreaseOfBatteryCell; samm:optional true; samm:payloadName "InternalResistanceIncreaseOfBatteryCell" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are InitialInternalResistanceOnBatteryModule / InternalResistanceIncreaseOfBatteryCell / InternalResistanceIncreaseOfBatteryPack dynamic data? If yes, are they correct in this data model or should they be part of batterypass.product_condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DIN DKE SPEC 99100 classifies them as static, this is why they are part of technical data and not of Product Condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, therefore the "initial", I get it. For fields like this I think an explanation like "initial value during manufacturing, not to be updated" might avoid misunderstandings. Thanks for the explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I unresolved the findings so that decription might be improved
io.admin-shell.idta.handover_documentation/2.0.0/gen/HandoverDocumentation.aas.json
Outdated
Show resolved
Hide resolved
io.admin-shell.idta.handover_documentation/2.0.0/gen/HandoverDocumentation.aas.json
Outdated
Show resolved
Hide resolved
io.admin-shell.idta.handover_documentation/2.0.0/gen/HandoverDocumentation.aas.json
Outdated
Show resolved
Hide resolved
"keys" : [ { | ||
"type" : "GlobalReference", | ||
"value" : "https://api.eclass-cdp.com/0173-1-02-ABI503-003" | ||
"value" : "file:///C:/Users/bsb2si/OneDrive%20-%20Bosch%20Group/__localWorkCloud/__git/admin-shell-io.smt-semantic-models/cli/0173-1%2302-ABI503%23003" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URL hard coded with Bosch in the string...
There are many hard coded URLs in this file I will not tag all of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there should be no new semantic Ids introduced. IDTA have already defined them for HandoverDocumentation. See also my comments here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bug in the generator and not the value as defined in the .ttl. The model itself seems to be correct. The bug is addressed in eclipse-esmf/esmf-sdk#766. Added to overview of known issues in this PR
|
||
: a samm:Namespace ; | ||
samm:preferredName "namespace for handover documentation"@en ; | ||
samm:description "This namespace is reserved for the Aspect Models and elements conformant to Submodel Template Specification (SMT) IDTA-02004 Handover Documentation, Version 2.0."@en . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A content-wise description is missing from my point of view. What documents should be put in here? Some documents seem to be required to be put into the CircularityBattery data model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tobzahn sorry, I do not understand: this is the namespace for handover documentation, not for Circulartiy. For ciruclarity there would be another namespace description. So far not all namespaces do have such a namespace.ttl: this is new in SAMM and I just wanted to test it whether it helps.
So in this namespace only .ttl for Handover Documentation that reflext the mentioned IDTA specification are contained.
Please check if the improved description for 6dd3612 helps. Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusion. Let me rephrase: To me, it is unclear how to use the HandoverDocumentation data model. I do not understand what can or needs to be put into the data model, therefore I think the description needs to be more clear. (In the diff you linked I did not see an updated description, but maybe I am just looking in the wrong place.)
Questions regarding the ProductCarbonFootprint data model:
I am asking because my understanding is that there need to be several different life cycle phases for all batteries, and since those phases are defined in the regulation this should also be defined in the data model. But if it is just a string everybody will use a different string. |
According to the regulation:
Therefore, I would expect to see an attribute called 'batteryStatus' in the product condition data model, with the possible attributes 'original', 'remanufactured', 'repurposed' or 'dismantled'. I found the attribute in the nameplate, but I assume batteryStatus is dynamic? |
In the battery regulation it is mentioned that the battery passport must include the carbon footprint performance class per Article 7 of the Battery Regulation. I was looking for a corresponding parameter in the CarbonFootprint data model, but I cannot find it. Is it missing or am I looking in the wrong place? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are more major bugs: There are inconsistencies in the submodels derived from the IDTA submodel original definitions such as DNP, PCF, Doc, Tech. These include semanticIds and shortIds usage (eg, shortIds they do not start with capital letters).
Is the "Draft Guidance on Data explanation required in EU battery passport" (CEN/TC 301/WG 18 "Electric vehicles batteries") already considered in this data model? |
Findings via PR #10
Major Bugs:
Minor Bugs:
Major changes:
<urn:samm:io.BatteryPass.Performance:1.2.1#>
instead of bp:<urn:samm:io.BatteryPass.Performance:1.2.0#>
<urn:samm:io.catenax.pcf:8.0.0#>
instead of cx:<urn:samm:io.catenax.pcf:7.0.0#>
Improvements:
Please be aware: some of the generated files also changed because of changes in reused Aspect Models from BatteryPass Consortium or Tractus-X/Catena-X, e.g.
-- batterypass/BatteryPassDataModel#23: remove editorial special char from descriptions
-- batterypass/BatteryPassDataModel#25: Circularity.ValidEmailAddress wrong regex
-- batterypass/BatteryPassDataModel#27: GeneralProductInformation:1.2.0 exampleValue for addressCountry is wrong
-- batterypass/BatteryPassDataModel#32: NumberOfFullCycles and others should be xsd:positiveInteger
-- batterypass/BatteryPassDataModel#31 ratedEnergy/CertifiedUsableBatteryEnergy: data type & no negative values #31
-- batterypass/BatteryPassDataModel#21 add descriptions to properties
-- batterypass/BatteryPassDataModel#20 optional properties? Add cardinality to Performance, MaeterialComposition and Circularity properties
-- batterypass/BatteryPassDataModel#19 Add example values to Performance, MaeterialComposition and Circularity properties
:kilogramperkilowatthour
to:KilogramPerKilowattHour
Open issues for generated files: