[VSS2] Attribute and signal as type

danielwilms commented 5 years ago

Current situation: Right now signals and attributes are modeled in the tree structure itself. Types can be either branch or data types. Some issues resolve from that modeling choice:

redundancy of tree structure in both branches
logically combined leafs are separated
graph type and data type mixed
private branch doesn't effect attribute tree
property in graph structure hard to change

Further there is #78 proposing a solution and #70 raising the issue.

Possible solution

Taken from #78 Attribute and Signal branch would be removed
Type would be changed from ['Branch', DataType1, ... n] to ['Branch','Signal','Attribute']
New element datatype would be introduced as element.

@UlfBj, could you check, if I forgot something?

UlfBj commented 5 years ago

Thank you for asking about my opinion.

I think you have got the arguments right for the current situation.

In the Possible solution below I think that the name ‘Signal’ is problematic. ‘Signal’ represents both the ‘Sensor’ and ‘Actuator’ types, as well as their combination ‘SenseActuator’ (my naming). I have also suggested that we should represent “streaming” data sources with a specific type ‘StreamSensor’. And that diagnostic type of data should have its own type, ‘Diagnostic’. With this said, I think that Type should be represented the set ['Branch', 'Sensor', 'Actuator', 'SenseActuator', 'StreamSensor', 'Diagnostic', 'Attribute']. The naming may be changed, but this is the set of types I think are relevant.

Further, if my contribution of the node types ‘Rbranch’ and ‘Element’ also should be taken into account, I guess they should also be added to the Type set.

I think it is a good idea to separate between Type and Datatype.

Ulf Björkengren Ph. D. Connectivity Strategist

gunnarx commented 5 years ago

I'd like to (help out to) get #78 merged as soon as possible so that things don't get out of date again from git point of view. #78 is still in conflict status and it requires a kind of special merge because of this -- based on Ulf's input we aim to essentially overwrite master with the content of #78. But that should be done technically using a set of git-merge steps if we want the history to include master rather "resetting" master to new commit, which is usually not done for commit histories that have been published). I have done this locally but want the group's input on whether this can be pushed onto master. Once it is done the future work becomes easier to do (from a pure git technical point of view).

danielwilms commented 5 years ago

@gunnarx: From a git perspective you're completely right. The change is huge and should be merged ASAP. Some things I still would like to consider to change.

I summarize from the discussion here:

attribute signal branch will be removed
datatype will be a new element with datatypes only
type will be ['Branch', 'Sensor', 'Actuator', 'SenseActuator', 'StreamSensor', 'Attribute']

One thing I'm not sure about is Diagnostic as a type, because at the end it's just a protocol which transmits the sensor value or acts on the actuator. Another is the term SenseActuator. I don't have a better suggestion right now, but it sounds a bit odd to mix a verb and a noun. Suggestions?

@UlfBj how shall we do it practically? Do you have today to work on this? Would you need support? Tomorrow night we have a call on the v2 changes, it would be cool to have the PR until then mergeable. I can help if needed.

UlfBj commented 5 years ago

Hi,

The proposed changes in bullets 1 to 3 is fine with me.

Regarding the Diagnostic type, my view is that e. g. DTC data does not so well fit into any of the available Type values in bullet 3. A VSS node SHALL have a Type, so which one in bullet 3 should be used for DTC tree nodes? But I do not have a strong opinion on this, it is fine to omit it.

I agree that SenseActuator is a somewhat odd name. I tried hard to find a better name, but I failed. However, I think this type needs a name of its own.

Regarding how we should do this practically, I am not the best to answer that, my Github knowledge is rather shallow. So I need help to do this. If Gunnar or someone else is willing to take the lead, I think that would be the best way forward. I can then be available to answer any questions regarding what I have tried to do in the pull request.

Please let me know how you think we should go about this.

It would be cool to have it ready for the meeting you mention☺.

BR Ulf

danielwilms commented 5 years ago

A VSS node SHALL have a Type, so which one in bullet 3 should be used for DTC tree nodes?

A DTC can be seen as an error on a specific functionality and maybe should be modeled there. Maybe we need some examples to see how we model this one.

If Gunnar or someone else is willing to take the lead, I think that would be the best way forward. I can then be available to answer any questions regarding what I have tried to do in the pull request.

I can take it from there. I'll see how much has to be changed the worst case, I'll just create a new one.

UlfBj commented 5 years ago

A DTC can be seen as an error on a specific functionality and maybe should be modeled there. Maybe we need some examples to see how we model this one. Ok.

I can take it from there. I'll see how much has to be changed the worst case, I'll just create a new one. Great. If any questions emerge that you think I may have an answer on, I am available most of the day via mail, or other means.

BR Ulf

gunnarx commented 5 years ago

A DTC can be seen as an error on a specific functionality and maybe should be modeled there. Maybe we need some examples to see how we model this one.

Ok.

I agree, but there will need to be a lot of examples, or it is likely to be modeled in a way that is not convenient. Some DTCs might be standard (e.g. described in a ISO diagnostics or AUTOSAR spec), but most of them are OEM specific so that is worth keeping in mind.

Besides, diagnostics involves a number of other aspects so I would suggest to not model this without consulting diagnostic specialists. There are more concepts such as freeze-frames and fault indications (signals). Before a DTC is set there may first be fault indications, and individual rules for each one.

Random example: This fault indication must exist for 7 seconds and then this DTC is set. If the fault signal later disappears for 1 hour, this DTC is cleared. But for another one the rule is that the DTC is never cleared. You get the idea. Now I realize that those rules are unlikely to be encoded in this database per se, I'm just saying the full picture should be understood before designing it.

A strategy could of course be started in terms of "ontology" how a DTC should relate to its function and to its signals. But that would be working on a general strategy, probably not yet ready to be put into a specific database until all details are known.

If we expect most DTCs to be OEM specific, then keeping them in a separate tree could make it a bit easier and it would "postpone" the more advanced design for a while.

gunnarx commented 5 years ago

@gunnarx: From a git perspective you're completely right. The change is huge and should be merged ASAP. Some things I still would like to consider to change.

Shall I make the merge to master now, or wait until that change discussion is done?

danielwilms commented 5 years ago

Shall I make the merge to master now, or wait until that change discussion is done?

Wait @gunnarx , I'll add some changes to the PR before.

rtroncy commented 5 years ago

3. type will be ['Branch', 'Sensor', 'Actuator', 'SenseActuator', 'StreamSensor', 'Attribute']

Regarding the list of possible types, as others have noted, I also agree that 'SenseActuator' is bizarre as naming. More importantly, why do you need it in a first place? To act the fact that some sensors may be both sensors and actuators? Multy-typing is fine, and generally well-implemented in schemadata and in particular ontologies. For examples, schema.org makes heavy use of Multi Typed Entities (MTE is a modeling pattern).

UlfBj commented 5 years ago

So you propose it would be expressed like this? ‘Type’ = [ ‘Sensor’, ‘Actuator’ ]

Adds a bit to implementation complexity, but makes the specification cleaner.

BR Ulf

UlfBj commented 5 years ago

Or could we say that ‘Actuator’ in almost all cases is equal to [ ‘Sensor’, ‘Actuator’ ], and handle the (few?) exceptions when the client tries to read a non-readable ‘Actuator’ with an error code?

rtroncy commented 5 years ago

So you propose it would be expressed like this? ‘Type’ = [ ‘Sensor’, ‘Actuator’ ] Adds a bit to implementation complexity, but makes the specification cleaner.

Yes, for some specific Type that are indeed both sensor and actuator.

danielwilms commented 5 years ago

So you propose it would be expressed like this? ‘Type’ = [ ‘Sensor’, ‘Actuator’ ] Adds a bit to implementation complexity, but makes the specification cleaner.

Yes, for some specific Type that are indeed both sensor and actuator.

Thanks for the input @rtroncy. The only problem I see is, that it adds complexity, as not every combination would be permitted (e.g. branch, attribute are mutually exclusive to the others). That makes me think as well, if StreamSensor is needed. Which update interval makes a sensor a streamsensor? To keep it simple, I would suggest: Branch: node in the tree Sensor: read-only which updates in some interval x Actuator: Sensor + write Attribute: read-only and static

What do you think?

UlfBj commented 5 years ago

My view on streaming sensor is not related to what update interval it has, but that it uses a streaming protocol. Thus this API can be used for camera/radar data streams by providing the url to the streaming server. The actual stream is not handled by this API. We discussed this in the W3C WG where it was backed up. BR Ulf

Skickat från min Samsung Galaxy-smartphone.

-------- Originalmeddelande -------- Från: danielwilms notifications@github.com Datum: 2018-11-28 19:45 (GMT+01:00) Till: GENIVI/vehicle_signal_specification vehicle_signal_specification@noreply.github.com Kopia: "Björkengren, Ulf" ulf.bjorkengren@volvocars.com, Mention mention@noreply.github.com Rubrik: Re: [GENIVI/vehicle_signal_specification] [VSS2] Attribute and signal as type (#79)

So you propose it would be expressed like this? ‘Type’ = [ ‘Sensor’, ‘Actuator’ ] Adds a bit to implementation complexity, but makes the specification cleaner.

Yes, for some specific Type that are indeed both sensor and actuator.

Thanks for the input @rtroncyhttps://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Frtroncy&data=02%7C01%7Culf.bjorkengren%40volvocars.com%7C34af563c667c4c7bb47308d655619e76%7C81fa766ea34948678bf4ab35e250a08f%7C0%7C0%7C636790275096010133&sdata=SvqJqrNnIOFfbaBC08bhoRFk4ZAioeozqO5mZ7B%2BLzA%3D&reserved=0. The only problem I see is, that it adds complexity, as not every combination would be permitted (e.g. branch, attribute are mutually exclusive to the others). That makes me think as well, if StreamSensor is needed. Which update interval makes a sensor a streamsensor? To keep it simple, I would suggest: Branch: node in the tree Sensor: read-only which updates in some interval x Actuator: Sensor + write Attribute: read-only and static

What do you think?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FGENIVI%2Fvehicle_signal_specification%2Fissues%2F79%23issuecomment-442559481&data=02%7C01%7Culf.bjorkengren%40volvocars.com%7C34af563c667c4c7bb47308d655619e76%7C81fa766ea34948678bf4ab35e250a08f%7C0%7C0%7C636790275096010133&sdata=9s5rswTNJyUoHzlm9AzuKzvEjtGdPUQ1B38h1d08F14%3D&reserved=0, or mute the threadhttps://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAkkYQYL-FM9k2E8zxwp3ZdjPM37-gi_5ks5uztmygaJpZM4Y1QkU&data=02%7C01%7Culf.bjorkengren%40volvocars.com%7C34af563c667c4c7bb47308d655619e76%7C81fa766ea34948678bf4ab35e250a08f%7C0%7C0%7C636790275096010133&sdata=wCuPE2lAVoZP7Elk27qVpxGkdy4R2x%2FYFZeNxMljixU%3D&reserved=0.

rtroncy commented 5 years ago

The only problem I see is, that it adds complexity, as not every combination would be permitted (e.g. branch, attribute are mutually exclusive to the others). That makes me think as well, if StreamSensor is needed. Which update interval makes a sensor a streamsensor? To keep it simple, I would suggest: Branch: node in the tree Sensor: read-only which updates in some interval x Actuator: Sensor + write Attribute: read-only and static

This is a good start. And I would add Stream as well for the case of @UlfBj mentioned. You may define your types as perfectly disjoint, as you seem to do when defining Sensor as read only and Actuator as sensors you can write. This means that there is no Actuator that is not a Sensor as well? As for enabling some combination of types (or not), this is a different issue if you follow the multi-typing modeling pattern and this can be constrained with shapes (or rules). In summary, it seems to me we agree on the top / main types so let's push this in the repo

danielwilms commented 5 years ago

My view on streaming sensor is not related to what update interval it has, but that it uses a streaming protocol.

Sorry, makes sense. Then I misunderstood.

In summary, it seems to me we agree on the top / main types so let's push this in the repo

Agreed. I will prepare it then.

zourongrong commented 5 years ago

3. type will be ['Branch', 'Sensor', 'Actuator', 'SenseActuator', 'StreamSensor', 'Attribute']

Should the 'SenseActuator' property be configured as: struct CanSignal geniviDemoSignals[] = { { .canId = 0x111, .sigId = 9, .sigName = "vehicle.engine.oilpressure", .in_start = 0, / signal bits produced by sensor / .in_end = 7, .out_start = 8, / signal bits consumed by sensor / .out_end = 15, .min = 0, .max = 160, .type = CAN_SIGNAL_UINT8, .function = SenseActuator, }, } I think the signals on CAN is single direction, so we use more bits to express a bidirection property?

danielwilms commented 5 years ago

@zourongrong, thanks for the feedback. The discussion went a bit away of the SenseActuator, even though the context behind it hasn't changed that much. First, I'd say that CAN is not the only target bus we have, but we should definitely consider the consequences there. Please checkout the previous comment above and let me know what you think.

danielwilms commented 5 years ago

@UlfBj I've done the changes and currently writing the documentation. There I came along the rbranch. Could you quickly help me out, what's the status with that one? This type should stay, am I right?

zourongrong commented 5 years ago

@danielwilms, thanks for your reply, my question is: Is there any example about how to use these data models? Maybe it shouldn't be discussed here, please ignore and sorry for that :).

UlfBj commented 5 years ago

Hi Daniel!

This has been thoroughly discussed in the W3C WG, where it was approved. So I would say that it should stay, together with the ‘element’ type. Naming could be discussed if a need is seen of that.

The reason for these two types is for VSS to support “resource” data model used in VIWI.

BR Ulf

danielwilms commented 5 years ago

@zourongrong, here's the perfect place to ask. I'm pushing the changes today and then we can discuss on the concrete documentation. I think that might be easier.

danielwilms commented 5 years ago

This has been thoroughly discussed in the W3C WG, where it was approved. So I would say that it should stay, together with the ‘element’ type. Naming could be discussed if a need is seen of that.

@UlfBj Thanks for the clarification. That's what I thought. Just wanted to be sure. :)

COVESA / vehicle_signal_specification

[VSS2] Attribute and signal as type #79