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.

Sign up for GitHub

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

Jump to bottom

Add pre-process step and a driver function to the test infrastructure #664

Open
wants to merge 4 commits into
base: staging
Choose a base branch
from
Open

Add pre-process step and a driver function to the test infrastructure #664

wants to merge 4 commits into from

Conversation

hydrolarus
Copy link
Contributor

@hydrolarus hydrolarus commented Oct 30, 2024 •
edited
Loading

This adds a pre-process and driver function to HITL tests as well as re-order some code.

@martijnbastiaan martijnbastiaan mentioned this pull request Nov 1, 2024
Open
@hydrolarus hydrolarus force-pushed the pre-process branch 7 times, most recently from 74b703c to 2b1f8f5 Compare November 6, 2024 15:29
@hydrolarus hydrolarus marked this pull request as ready for review November 6, 2024 15:29
hydrolarus
hydrolarus commented Nov 6, 2024
View reviewed changes
bittide-shake/exe/Main.hs Outdated Show resolved Hide resolved
@hydrolarus
Copy link
Contributor Author

Since this rewrites all the tests using VexRiscv to be programmed before HITL test start it needs to access all FPGAs via JTAG, but #649 blocks this, so the software clock control HITL test is blocked on that.

@martijnbastiaan martijnbastiaan self-requested a review November 13, 2024 14:18
@martijnbastiaan martijnbastiaan added the 2 Review/merge second label Nov 13, 2024
@hiddemoll
Copy link
Contributor

PR #649 has been closed in favor of #675 , which has been merged last Friday.

@hydrolarus hydrolarus force-pushed the pre-process branch 5 times, most recently from 76d7cbe to 51e6ae4 Compare November 26, 2024 12:14
@hydrolarus hydrolarus force-pushed the pre-process branch 3 times, most recently from f26a03a to d557c16 Compare November 27, 2024 15:19
martijnbastiaan
martijnbastiaan requested changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@martijnbastiaan martijnbastiaan left a comment

Choose a reason for hiding this comment

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

Very nice!

My biggest concern is use of manual cleanup calls. We should make sure we clean up properly, even in case of errors --- hence the need for bracket style resource allocation. Other than that LGTM.

bittide-experiments/src/Bittide/Hitl.hs
Comment on lines +107 to +112
data DeviceInfo = DeviceInfo
{ deviceId :: String
, dna :: BitVector 96
, serial :: String
, usbAdapterLocation :: String
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add some example strings to the field docs?

bittide-experiments/src/Bittide/Hitl.hs Outdated
Comment on lines 237 to 240
| CustomPreProcess
(VivadoHandle -> FilePath -> HwTarget -> DeviceInfo -> IO (TestStepResult c))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this override or is this an additional step? Could you add that to the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does an override, an additional step would have a different signature and be more complex (how does the driver receive the user-data from multiple different pre-process functions?).

I'll add that to the docs!

bittide-instances/src/Bittide/Instances/Hitl/Driver/VexRiscv.hs
let targetIndex = fromMaybe 9 $ L.findIndex (\d -> d.deviceId == targetId) demoRigInfo
let gdbPort = 3333 + targetIndex

(ocd, ocdClean) <- startOpenOcd deviceInfo.usbAdapterLocation gdbPort
Copy link
Contributor

Choose a reason for hiding this comment

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

I very much want to discourage manual resource cleaning. Bracket-style is the way to go. I.e., use withBlaBla functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree generally. Here the "issue" is that the pre-process step might want to open some programs and then make them available in the driver function. If a bracket-style function is used to start programs then there's no way to pass them through to the driver function (for example opening GDB to program the core, then later interacting with it using GDB commands)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh... okay I don't have a great solution right now, but be should definitely make an issue out of it.

vivado-hs/src/Vivado/Tcl.hs
Comment on lines +23 to +24
openHwT :: VivadoHandle -> HwTarget -> IO ()
openHwT v hwT = do
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
openHwT :: VivadoHandle -> HwTarget -> IO ()
openHwT v hwT = do
openHwTarget :: VivadoHandle -> HwTarget -> IO ()
openHwTarget v hwT = do

@hydrolarus hydrolarus force-pushed the pre-process branch 4 times, most recently from c6286a7 to 55c8ca2 Compare December 3, 2024 12:52
martijnbastiaan
martijnbastiaan approved these changes Dec 4, 2024
View reviewed changes
Copy link
Contributor

@martijnbastiaan martijnbastiaan left a comment

Choose a reason for hiding this comment

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

Let's make an issue out of resource allocation. Other than the small comments, LGTM. Can we help debugging the failing CI?

@rslawson
Copy link
Collaborator

rslawson commented Dec 4, 2024

Just rebased on the most recent staging (:

@rslawson rslawson force-pushed the pre-process branch 3 times, most recently from e585db4 to 086ce0f Compare December 4, 2024 15:29
@hydrolarus hydrolarus force-pushed the pre-process branch 2 times, most recently from 70344b7 to b4bb20c Compare December 11, 2024 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martijnbastiaan martijnbastiaan martijnbastiaan approved these changes

@rslawson rslawson Awaiting requested review from rslawson

Assignees
No one assigned
Labels
2 Review/merge second
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hydrolarus @hiddemoll @rslawson @martijnbastiaan