ec-test-app: Add serial source #38
Conversation
kurtjd
requested review from
dymk,
felipebalbi,
philgweber,
tullom and
williampMSFT
philgweber
left a comment
philgweber
left a comment
There was a problem hiding this comment.
Will have to think about the transport and integrating with FFA and secure world so we have the ability to redirect messages through either path.
So I was thinking this can be used as an alternative to going through FFA when someone jsut wants to quickly connect to a board and do some testing of the services. In my mind I imagined this app will just select the |
kurtjd
force-pushed
the
add-serial-source
branch
from
72c2645 to
75bdfa4
Compare
kurtjd
force-pushed
the
add-serial-source
branch
from
75bdfa4 to
00a491e
Compare
| // ODP | ||
| buffer[8] = 0x7D; | ||
| buffer[9] = 1 << 1; | ||
| buffer[10] = dst.into(); |
There was a problem hiding this comment.
I don't think this header format is correct - I had to tweak it as part of the change to break out comms types for each service: OpenDevicePartnership/embedded-services#670 - see https://github.com/OpenDevicePartnership/embedded-services/blob/e2a466fbaaa4bdd1e26ce4d5b226e8722303d214/embedded-service/src/relay/mod.rs#L290
Incidentally, can we get this from mctp-rs rather than hand-rolling packet headers like this?
There was a problem hiding this comment.
Hm I got these from your PR here: https://github.com/philgweber/n1x-ec-secure-partition/pull/1/files#diff-bb5401a4bb778df1bfeef464cdf7aa6a622473b45dd11ab0480f4f74675928d5
Are these outdated? Things seem to be working fine with no issues so far, but I can take another look.
As far as mctp-rs, unfortunately the crate only supports deserializing requests and serializing responses from what I can see, and not the reverse which is what we would need here.
There was a problem hiding this comment.
Oh, whoops, sorry, I was thinking about it from the other side of the wire where the 'header' includes distinct service IDs and message IDs. That PR was an attempt to hack the existing hack to work with the new header, but I think the plan might be to deprecate that whole thing so I didn't invest in switching to mctp-rs. If you're populating the message ID after this 'header' then you should be fine.
I'm not sure I follow re: only serializing requests/deserializing responses, though - can't we just switch the types we're using (i.e. tell mctp-rs that the 'response' type is the request type and vice-versa)?
There was a problem hiding this comment.
To be honest I didn't look too deeply, but I saw this line: https://github.com/dymk/mctp-rs/blob/f3121512468e4776c4b1d2d648b54c7271b97bd9/src/serialize.rs#L10 which made me think serialization only works for responses/replies since it uses the reply_context term, but maybe it doesn't matter heh. I can give it a try anyway.
There was a problem hiding this comment.
@williampMSFT Can this wait until a future point? It seems in general there is still some confusion among the team about some of the comms stuff and once it's cleared up this can be revisited. This at least works for now and will help unblock folks who need to test host to EC comms. I've created an issue for it here: #40
There was a problem hiding this comment.
Eh after taking a closer look it should be perfectly feasible. Will get that done today. Was also thinking best how to share the OdpHeader and friends between the uart-service and this app but may have to revisit that. I think I could just make those types generated by the macro pub instead of pub(crate) so I can use them here like uart_service::mctp::OdpHeader.
There was a problem hiding this comment.
Could extract to a crate if its own and use it as dependency for everyone.
There was a problem hiding this comment.
Yeah at least for the SMBUS and MCTP header the mctp-rs crate should be able to handle that. For the ODP header last I spoke with Billy he made a good point that it shouldn't necessarily be its own crate since it will be defined by OEMs (via the macro call).
Currently I call the macro directly in the uart service (like the espi service) but I think the goal is to revisit it such that the espi and uart services can be passed this information from the OEM. Hence for now until some of that is figured out might just stick with the manual serialization of the ODP header.
There was a problem hiding this comment.
After further attempting to do this, I've come to the conclusion there are a number of little things that need to change here and there in our comms system that I don't have the time at the moment to solve cleanly, so will keep this as is for the time-being.
kurtjd
force-pushed
the
add-serial-source
branch
from
00a491e to
291ab9f
Compare
| fn charge_state_as_str(state: BatteryState) -> &'static str { | ||
| if state.contains(BatteryState::DISCHARGING) { | ||
| "Discharging" | ||
| } else { | ||
| "Charging" |
There was a problem hiding this comment.
impl Display for BatteryState?
There was a problem hiding this comment.
BatteryState is a foreign type and Display is a foreign trait so orphan rule wouldn't allow that, right? But can give it a try.
There was a problem hiding this comment.
Where is battery state defined? Could impl Display on that crate and just rely on it here.
There was a problem hiding this comment.
It's defined by the embedded-batteries crate which is reexported by the battery-service-messages crate. Was also missing a couple TryFrom<u32>s in there which I opened a PR for so I can do that for these as well. However, initial attempts to update embedded-batteries throughout caused a lot of issues so ideally would like this to get in first then can take the time to fix everything that an embedded-batteries update will cause.
4 participants
This PR does a few things:
Source, which allows the app to talk directly to a dev platform in user-space (over a COM port for example). Can be useful for testing services without needing to worry about FFA and whatnot.Tested this on the
dev-imxrtplatform and works pretty well. I was running into some issues where I occasionally receive bytes that shouldn't be coming in for reasons I haven't been able to figure out yet, but if we drain the incoming bytes and just return an error it leaves things in a good state for the next attempt and we quickly recover.