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.

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

FSM state diagram #306

Merged
merged 11 commits into from
Jan 23, 2025
Merged

FSM state diagram #306

merged 11 commits into from
Jan 23, 2025

Conversation

jamesturner246
Copy link
Contributor

@jamesturner246 jamesturner246 commented Jan 14, 2025

Description

Add a flowchart representing the FSM state to the controller main page.

We use Mermaid JS to generate the flowchart on the state_machine page in controller. This graph is generated and updated dynamically based on what is returned by get_fsm_architecture() and get_fsm_state.

The current state and legal transitions are labelled green. All else grey.

Some basic styling is added to match the theme better.

Fixes #300

Type of change

  • Documentation (non-breaking change that adds or improves the documentation)
  • New feature (non-breaking change which adds functionality)
  • Optimization (non-breaking, back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (whatever its nature)

Key checklist

  • All tests pass (eg. python -m pytest)
  • The documentation builds and looks OK (eg. python -m sphinx -b html docs docs/build)
  • Pre-commit hooks run successfully (eg. pre-commit run --all-files)

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added or an issue has been opened to tackle that in the future. (Indicate issue here: # (issue))

@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.31%. Comparing base (bdcebf8) to head (c7bc7c0).
Report is 43 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #306      +/-   ##
==========================================
+ Coverage   85.93%   86.31%   +0.37%     
==========================================
  Files          38       38              
  Lines         583      599      +16     
==========================================
+ Hits          501      517      +16     
  Misses         82       82              

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

@jamesturner246 jamesturner246 marked this pull request as ready for review January 20, 2025 08:07
Copy link
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

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

I have a couple of comments/question, and a request.

Having a proper diagram is nice, but currently, the look and feel and the position at the bottom of the page is not that great. I get this:

image

The diagram is tiny, hard to read and, for some reason, makes the column with the FSM stuff wider than it should, with extra blank space at the right for no reason because the diagram does not extend that far. A few suggestions would be:

  • Try to make the diagram bigger, and maybe horizontal, and at the top, rather than the bottom.
  • Whatever the orientation, make sure the table is as wide as the FSM block.
  • Use dark blue font of the active state.
  • For the transitions, use transparent/white background, dark blue fonts for the active ones and grey for the not active ones.

I am not sure if any of these suggestions would make it any better, but as it is now, it is not that readable.

@jamesturner246
Copy link
Contributor Author

jamesturner246 commented Jan 21, 2025

The diagram is tiny, hard to read and, for some reason, makes the column with the FSM stuff wider than it should, with extra blank space at the right for no reason because the diagram does not extend that far. A few suggestions would be:

Yeah, the diagram is supposed to be in a column to the right of the FSM table, but it doesn't look great for small screens.

However, I'm a bit hesitant to mess around with the layout, given @cvzbynek will be working on the controller page layout in #301. Perhaps we can review layout, and TD or LR in that PR once done?

@dalonsoa
Copy link
Collaborator

I would not waste too much time with the layout, but I would definitely give it another pass. As it sits now, it's really neither nice nor useful. My screen is not small, but 4 columns side by side might be way too much for any sensible screen.

@cvzbynek
Copy link
Collaborator

There is a sneak peak of my version of layout. Don't sweat it. We will discuss it as my changes go to PR. @dalonsoa you can have a look yourself in branch Controller_UI_rework
image

@jamesturner246
Copy link
Contributor Author

jamesturner246 commented Jan 21, 2025

Thanks @cvzbynek.

Okay, I've implemented your suggestions where possible @dalonsoa. Diagram is at top, and highlighted node font colour is dark blue. It may well be changed soon, but it hopefully looks alright for now.

@jamesturner246
Copy link
Contributor Author

Screenshot from 2025-01-21 18-54-59

Copy link
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

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

I think the chart at the top looks way, way nicer and readable, and definitely LR works better. Great work!

@jamesturner246 jamesturner246 merged commit 0c16d68 into main Jan 23, 2025
4 checks passed
@jamesturner246 jamesturner246 deleted the fsm_diagram branch January 23, 2025 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider adding a diagram of the FSM to the Controller UI
4 participants