search

lf-edge / eve-api

Repository for eve-api code
Apache License 2.0
0 stars 13 forks source link

devmodelcommon.proto: Add types for CAN devices #44

Closed rene closed 8 months ago

rene commented 8 months ago

devmodelcommon.proto: Add types for CAN devices

Extended the API to support CAN Devices on EVE. The proposal is described and discussed at LFEdge's EVE Wiki:

https://wiki.lfedge.org/display/EVE/CAN+Bus+support

Three new members are added to PhyIoType:

Signed-off-by: Renê de Souza Pinto rene@renesp.com.br

rene commented 8 months ago

We should ignore the buflint complaints about using PHY_IOTYPE prefix etc since it makes more sense to follow the style for the existing enum values.

But a question. Would a controller (and associated UI etc) want to know whether a device understands these new enum values before sending them in the API? If so we should also add a new value in the API Capabilities enum in the API for CAN support. But if we don't think the controller cares then that isn't needed.

@eriknordmark , AFAU receiving a non supported I/O type is harmless because pillar will not perform any special handling of it... From what I see in the API we don't treat every single supported I/O type in capabilities, so I think we don't need any special treatment for CAN devices... but I don't have the full picture of the API, so I'm totally OK to accept the approach you and others point here as more adequate for this particular case...

eriknordmark commented 8 months ago

@eriknordmark , AFAU receiving a non supported I/O type is harmless because pillar will not perform any special handling of it... From what I see in the API we don't treat every single supported I/O type in capabilities, so I think we don't need any special treatment for CAN devices... but I don't have the full picture of the API, so I'm totally OK to accept the approach you and others point here as more adequate for this particular case...

Does the EVE code not care if the I/O type is 17 (some undefined value) and still look at the pci addresses etc? If so we don't need a API capability.

rene commented 8 months ago

@eriknordmark , AFAU receiving a non supported I/O type is harmless because pillar will not perform any special handling of it... From what I see in the API we don't treat every single supported I/O type in capabilities, so I think we don't need any special treatment for CAN devices... but I don't have the full picture of the API, so I'm totally OK to accept the approach you and others point here as more adequate for this particular case...

Does the EVE code not care if the I/O type is 17 (some undefined value) and still look at the pci addresses etc? If so we don't need a API capability.

I looked at domain manager's code and found basically two main functions that could potential fail due to an unknown IoType: https://github.com/lf-edge/eve/blob/master/pkg/pillar/cmd/domainmgr/domainmgr.go#L2763 and https://github.com/lf-edge/eve/blob/master/pkg/pillar/types/assignableadapters.go#L186

In the latter, there is this conversion ib.Type = IoType(phyAdapter.Ptype) , for unknown IoTypes, it will keep the phyAdapter.Ptype, i.e., any number above the known types, like 17, 18, etc...

There are a lot of checks for specific types, as I mentioned, but I couldn't find any returns or errors generated due to an unknown type... so I think we are safe....

rene commented 8 months ago

@eriknordmark , btw I've found this https://github.com/lf-edge/eve/blob/master/pkg/pillar/types/assignableadapters.go#L237 :

        IoNVMEStorage IoType = 9
    IoSATAStorage IoType = 10
    IoNetEthPF    IoType = 11
    IoNetEthVF    IoType = 12
    IoNVME        IoType = 255
    IoOther       IoType = 255

IoNVME has the same value of IoOther.... I don't think that's on purpose, it is?

eriknordmark commented 8 months ago

There are a lot of checks for specific types, as I mentioned, but I couldn't find any returns or errors generated due to an unknown type... so I think we are safe....

Thanks for checking.

eriknordmark commented 8 months ago

@eriknordmark , btw I've found this https://github.com/lf-edge/eve/blob/master/pkg/pillar/types/assignableadapters.go#L237 :

        IoNVMEStorage IoType = 9
  IoSATAStorage IoType = 10
  IoNetEthPF    IoType = 11
  IoNetEthVF    IoType = 12
  IoNVME        IoType = 255
  IoOther       IoType = 255

IoNVME has the same value of IoOther.... I don't think that's on purpose, it is?

That was the purpose initially. But now we have the unique type IoNVMEStorage so this should be removed AFAIR. However, I see code in pkg/pillar/cmd/domainmgr/domainmgr.go which refers to IoNVME and not IoNVMEStorage. Can you check with Ioannis? If so he can remove it from the API and replace the usage in domainmgr.