add the docs to make a new device #579

stan-dot · 2024-05-28T16:30:56Z

Fixes #578

Instructions to reviewer on how to test:

Do thing x
Confirm thing y happens

Checks for reviewer

Would the PR title make sense to a scientist on a set of release notes
If a new device has been added does it follow the standards
If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

stan-dot · 2024-05-28T16:31:39Z

the tests are failing because of an ophyd-async issue

callumforrester

I like the idea, but I'm not sure flowcharts are the best format for this. Read my other comments and see what you think.

docs/developer/how-to/make-new-ophyd-async-device.md

callumforrester · 2024-06-03T07:54:04Z

docs/developer/how-to/make-new-ophyd-async-device.md

+  flowchart TD
+    highLevelStart([Start]) --> scoutUsers[Scout current and potential users of the device, tag them in the draft PR]
+    scoutUsers --> testPVs[Test PVs to get the right values]
+    testPVs --> decideStateEnums[Decide on State Enums: extend str, Enum]
+    decideStateEnums --> chooseMethods[Choose which superclass methods to override]
+    chooseMethods --> finalizeDevice[Finalize Device Implementation]
+    finalizeDevice --> requestFeedback[Request feedback from tagged users]
+    requestFeedback --> rebaseOnMain[Rebase on main -> make sure tests pass still]
+    rebaseOnMain --> coordinateMerges[Coordinate merges with other codebase updates]
+    coordinateMerges --> markPRReady[Mark PR as ready]


Could: I'm actually not sure this works very well as a flowchart for the following reasons:

It prescribes an arbitrary order in which to do things, who says I should test the PVs before coming up with enum values? The other way around is actually easier using the dodal CLI.

It implies you should do each step once and never again. In reality making a device is nothing like that, it is an iterative process. The dodal CLI makes it easy to constantly test the PVs as you write signals, rather than once upfront. You also don't come up with a list of enums upfront, you work through one signal at a time and when you discover one that needs to be an enum, you write it and decide on enum values for that signal only, rinse and repeat.

It takes up a lot of page-space to convey comparatively little information.

I think what we want here is a worked example, rather than a precise series of steps. Perhaps a paragraph explaining in prose about testing the PVs, making enum signals, raising draft PRs etc. and then an example device with all of those things?

I agree with Callum, I think a worked example would be better

It takes up a lot of page-space to convey comparatively little information.
1 I got this based on #480 experiences where defining enum values before testing the right PVs was counterproductive. I'm happy to change this into a command from dodal cli, I didn't know this existed
2 then we can change it for there to be an iterative process. we can put what you just wrote.
page-space is free and it's not like we're printing this. empty space is the key to clear design.

re 'worked example': it's personal preference, it's fair that some people prefer prose modality to a diagram or the other way around, I think for inclusiveness we should support both.

I would argue they are fundamentally different. My suggestion is a rough guide to help you figure out the sequence of steps you need to take. The current flowchart provides a precise sequence of steps, which I think is too prescriptive.

Of the 3 flowcharts, only one has branching while the others are a strict linear progression. I agree that those make more sense as a worked example.

@callumforrester pls what would be a good prose exmple here?

This but with more words/detail:

"Familiarise yourself with the basics of ophyd-async devices , some considerations you should make while developing are , see example below. You are advised to gather requirements early, raise draft PRs, seek feedback, make use of the CLI to test. You should try to consolidate similar devices, the tests give you confidence that they will still work if you change them
".

The current flowchart provides a precise sequence of steps, which I think is too prescriptive.

this is just a guide and of course every user will need to adapt this. word-by-word maybe less than 5% of devices would have gone through this process in this step order.

I don't see this as 'the order' to do things, just 'an order of things to base your investigations on that roughly should work if your use case is the average one'.

and I'm not sure how many more words/detail you see fit, this is very vague to me maybe I am not the best person to write out the prose part.

docs/developer/how-to/make-new-ophyd-async-device.md

callumforrester · 2024-06-03T07:54:54Z

docs/developer/how-to/make-new-ophyd-async-device.md

+    testStart([Start Testing]) --> createFixtures[Create fixtures for various states of the device]
+    createFixtures --> createMockReactions[Create mock reactions to signals]
+    createMockReactions --> testStateTransitions[Test each device state transition]
+    testStateTransitions --> testAgainstHardware[Test against hardware]
+    testAgainstHardware --> testingComplete[Testing Complete]


Ditto to my other comment, I would prefer examples to flowcharts

callumforrester · 2024-06-03T07:59:56Z

docs/developer/how-to/make-new-ophyd-async-device.md

Could: Extra things that I think should also go in here:

The objective is to get it into main and then test it on the beamline! Ensuring fast iteration. You write the device, you assume you know how it works, you write tests against those assumptions, you merge. Then and only then do you test your assumptions on the beamline. If they were wrong, that's more issues.

See if you can find a similar device that someone has already made. If one seems to exists but has different PV names, those can and should be changed. Advise consulting with controls. If the device is urgently needed, two copies can coexist but there must be an issue to reconcile.

There's some of this already but more general advise not to do this task alone. Consult before starting, seek feedback early (e.g. draft PR) and merge with other people's devices where possible. The test suite should provide the confidence to do so without breaking anything.

DiamondJoseph · 2024-06-03T09:15:21Z

docs/developer/how-to/make-new-ophyd-async-device.md

+
+This high-level flowchart guides the overall process of creating a new ophyd_async device, from testing PVs to overriding methods and determining state enums.
+
+```{mermaid}


Suggested change

```{mermaid}

```mermaid

Should get this to render?

idk, for me it renders in vscode preview

DiamondJoseph · 2024-06-03T09:15:33Z

docs/developer/how-to/make-new-ophyd-async-device.md

+
+This flowchart assists in selecting the appropriate interfaces based on the device's capabilities, such as file writing, reading values from process variables (PVs), and mobility within scans.
+
+```{mermaid}


Suggested change

```{mermaid}

```mermaid

DiamondJoseph · 2024-06-03T09:15:43Z

docs/developer/how-to/make-new-ophyd-async-device.md

+
+This flowchart outlines the testing procedure for the new ophyd_async device, from creating fixtures to testing state transitions and hardware integration.
+
+```{mermaid}


Suggested change

```{mermaid}

```mermaid

DiamondJoseph · 2024-06-03T09:18:18Z

docs/developer/how-to/make-new-ophyd-async-device.md

+  flowchart TD
+    highLevelStart([Start]) --> scoutUsers[Scout current and potential users of the device, tag them in the draft PR]
+    scoutUsers --> testPVs[Test PVs to get the right values]
+    testPVs --> decideStateEnums[Decide on State Enums: extend str, Enum]
+    decideStateEnums --> chooseMethods[Choose which superclass methods to override]
+    chooseMethods --> finalizeDevice[Finalize Device Implementation]
+    finalizeDevice --> requestFeedback[Request feedback from tagged users]
+    requestFeedback --> rebaseOnMain[Rebase on main -> make sure tests pass still]
+    rebaseOnMain --> coordinateMerges[Coordinate merges with other codebase updates]
+    coordinateMerges --> markPRReady[Mark PR as ready]


Of the 3 flowcharts, only one has branching while the others are a strict linear progression. I agree that those make more sense as a worked example.

codecov · 2024-06-13T09:28:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.69%. Comparing base (76be228) to head (55d69fa).
Report is 24 commits behind head on main.

❗ Current head 55d69fa differs from pull request most recent head f133119

Please upload reports for the commit f133119 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #579      +/-   ##
==========================================
+ Coverage   93.03%   93.69%   +0.66%     
==========================================
  Files         103      104       +1     
  Lines        3903     3967      +64     
==========================================
+ Hits         3631     3717      +86     
+ Misses        272      250      -22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

DiamondJoseph

There's no worked examples in this repository of how to produce a device. I think it makes a lot of sense to defer that to existing ophyd-async docs (& docs).

Some of the process of writing a device should then be captured there (enum validation, testing PVs) , and then this can be simplified to "when to create/not create a new device", "how to write a good issue that captures requirements" and "how to properly test a device".

DominicOram

I still think that this would be better as a worked example but it's a reasonable start

stan-dot requested review from DominicOram, callumforrester and DiamondJoseph May 28, 2024 16:30

stan-dot self-assigned this May 28, 2024

stan-dot linked an issue May 28, 2024 that may be closed by this pull request

Add a flowchart on the decisions to make a new ophyd device for DLS #578

Closed

stan-dot added the documentation Improvements or additions to documentation label May 28, 2024

stan-dot force-pushed the 578-add-a-flowchart-on-the-decisions-to-make-a-new-ophyd-device-for-dls branch from b7ae8df to a2dd6c3 Compare May 28, 2024 16:31

stan-dot force-pushed the 578-add-a-flowchart-on-the-decisions-to-make-a-new-ophyd-device-for-dls branch 2 times, most recently from 46ea122 to 3b84edb Compare June 3, 2024 07:17

callumforrester requested changes Jun 3, 2024

View reviewed changes

DiamondJoseph reviewed Jun 3, 2024

View reviewed changes

stan-dot requested review from callumforrester and DiamondJoseph June 3, 2024 10:41

stan-dot force-pushed the 578-add-a-flowchart-on-the-decisions-to-make-a-new-ophyd-device-for-dls branch 7 times, most recently from 8057efc to ced0a10 Compare June 12, 2024 10:43

stan-dot force-pushed the 578-add-a-flowchart-on-the-decisions-to-make-a-new-ophyd-device-for-dls branch 3 times, most recently from e90f489 to 55d69fa Compare June 13, 2024 09:25

stan-dot force-pushed the 578-add-a-flowchart-on-the-decisions-to-make-a-new-ophyd-device-for-dls branch 5 times, most recently from ab23442 to bab8edd Compare June 18, 2024 07:57

DiamondJoseph reviewed Jun 18, 2024

View reviewed changes

DominicOram approved these changes Jun 18, 2024

View reviewed changes

stan-dot force-pushed the 578-add-a-flowchart-on-the-decisions-to-make-a-new-ophyd-device-for-dls branch from bab8edd to 86b1348 Compare June 18, 2024 10:16

stan-dot mentioned this pull request Jun 18, 2024

Add an example to showcase device creation process #634

Open

callumforrester approved these changes Jun 18, 2024

View reviewed changes

stan-dot added 4 commits June 18, 2024 15:27

add the docs to make a new device

b9bcba7

update following some suggestions

78c6598

more realistic testing chart

0316fad

respond to comments

f133119

stan-dot force-pushed the 578-add-a-flowchart-on-the-decisions-to-make-a-new-ophyd-device-for-dls branch from 86b1348 to f133119 Compare June 18, 2024 14:27

stan-dot merged commit 1829309 into main Jun 18, 2024
11 checks passed

stan-dot deleted the 578-add-a-flowchart-on-the-decisions-to-make-a-new-ophyd-device-for-dls branch June 18, 2024 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add the docs to make a new device #579

add the docs to make a new device #579

stan-dot commented May 28, 2024 •

edited

Loading

stan-dot commented May 28, 2024

callumforrester left a comment

callumforrester Jun 3, 2024

DominicOram Jun 3, 2024

stan-dot Jun 3, 2024

callumforrester Jun 3, 2024

DiamondJoseph Jun 3, 2024

stan-dot Jun 3, 2024

callumforrester Jun 6, 2024

stan-dot Jun 6, 2024

callumforrester Jun 3, 2024

callumforrester Jun 3, 2024

DiamondJoseph Jun 3, 2024

stan-dot Jun 3, 2024

DiamondJoseph Jun 3, 2024

DiamondJoseph Jun 3, 2024

DiamondJoseph Jun 3, 2024

codecov bot commented Jun 13, 2024 •

edited

Loading

DiamondJoseph left a comment

DominicOram left a comment


		This high-level flowchart guides the overall process of creating a new ophyd_async device, from testing PVs to overriding methods and determining state enums.

		```{mermaid}


		This flowchart assists in selecting the appropriate interfaces based on the device's capabilities, such as file writing, reading values from process variables (PVs), and mobility within scans.

		```{mermaid}


		This flowchart outlines the testing procedure for the new ophyd_async device, from creating fixtures to testing state transitions and hardware integration.

		```{mermaid}

add the docs to make a new device #579

add the docs to make a new device #579

Conversation

stan-dot commented May 28, 2024 • edited Loading

Instructions to reviewer on how to test:

Checks for reviewer

stan-dot commented May 28, 2024

callumforrester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 13, 2024 • edited Loading

Codecov Report

DiamondJoseph left a comment

Choose a reason for hiding this comment

DominicOram left a comment

Choose a reason for hiding this comment

stan-dot commented May 28, 2024 •

edited

Loading

codecov bot commented Jun 13, 2024 •

edited

Loading