Skip to content

Conversation

@gerth2
Copy link
Contributor

@gerth2 gerth2 commented May 5, 2022

Since there's a decent amount of tearup here, starting a draft PR early to gather/document comments.

Core Goals

  • Revise advanced controls article to introduce concepts in a zero-to-advanced progression.
  • Clearly illustrate the "minimum viable product" techniques, why they do or don't work, and what the "next-step" should be.
  • Emphasize intuition of why certain things should and shouldn't work, feathering in more and more math as students progress toward more advanced techniques.
    • Secondairly, this intuition should help students improve their debugging abilities - knowing what symptoms are likely based in bad controller design or tune, or what's just a software bug or mechanical/electrical issue.
  • Present feed-forward first, rather than the traditional pedagogy of PID/feedback first.
  • Pictures tell a thousand words

Technical Changes

Add sufficient js infrastructure to support interactive visualizations of sample mechanisms:

  • Flywheel with single ball injected in, plus friction effects and sensor delay
    • Good for teaching bang-bang, feedback-only, and showing how they get close but have downsides
    • Good for teaching kS/kV/kA model for motors, which accounts for defficencies in feedback-only
    • Has potential to show motion profiling basics? Not sure if this is worthwhile though
  • Vertical arm, impacted by gravity
    • Good to show how bang bang is completely insufficent for the application
    • Good to show how feedback can help, but will still have limitations
    • Great to show how a non-linear feed-forward can correct for gravity
    • Great for showing motion profiling

Source for the sim for the flywheel is here, and for the arm is here

To support this, we'd need:

  • One or more new custom .js files, included into the sphinx build.
    • Tentative - source aggregation and minimization at page build time?
    • Tentative - only include the file in the pages that need it, rather than everywhere?
  • New dependency to Highcharts
    • I do have a contact here who blessed our team usage of it for FRC. They'd probably like it if we wrote them another blog post after we get this out, if we do indeed use their product
    • Per their webpage, usage for open-source projects should be free:
      • image
    • For now I just downloaded their latest release and dropped the .js file into the frc-docs repo.

Structural Changes

  • Add additional verbiage to the intro doc to explain the "burning platform" why teams might want to look into controls
  • Add links and "signpost" verbiage into each page to make it clear the docs are designed to be a progression from basic to advanced
  • Re-organize content to not introduce concepts before they are needed
  • Make it clear what each technique is capable of doing, and where the limits are
  • Ensure students are encouraged to pattern-match what they see in the docs to what they see on their particular robot
  • Add auto-play mp4's with clips from robots that have controls-oriented mechanisms, for maximum inspiration.
  • Add walkthrough tuning exercises to encourage students to play with different plant models and controllers, to see how they're expected to behave.

Status

May 5 2022 - Still very much in flight. Content is representative of the direction I want things to take, but far from complete or set in stone. Will update this PR when things are ready for detail review. In the mean time, comment away with concerns - it'll be easier to adjust course now, rather than in 5 months.

See comments below for latest status.

@Daltz333
Copy link
Member

Daltz333 commented May 8, 2022

Pictures tell a thousand words

Absolutely. The more pictures and visualizations, the better.

One or more new custom .js files, included into the sphinx build.
Tentative - source aggregation and minimization at page build time?
Tentative - only include the file in the pages that need it, rather than everywhere?

I'll look into only deploying the JS during build time, and I believe we have a script that minimizes at build time already. If not, it can be easily arranged. How large is the JavaScript?

Another consider I have is mobile usage. Over half of our viewers are using mobile devices and we want to keep that in mind.

Lastly, this is an excellent and must-needed contribution. I do want to make the glossary feature aware, and that it should be used quite liberally. We have hover tips enabled on glossary entries and it makes tracing the vocabulary so much easier.

Copy link
Member

@Daltz333 Daltz333 left a comment

Choose a reason for hiding this comment

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

To keep things standard and consistent, we have the following style constraints.

  1. Articles with spaces in the name should be in the format this-is-my-article. Instead of this_is_my_article
  2. You should only have one blank line between paragraphs and sections. Remember that source readability is just as important as output.
  3. Section underlines go in the order of =, -, ^, ~. If this you are using more than this, then the article should need to be restructured.

gerth2 and others added 2 commits May 8, 2022 13:20
…_to_ctrl_sys_design.rst

Co-authored-by: Dalton Smith <daltzsmith@gmail.com>
…_to_ctrl_sys_design.rst

Co-authored-by: Dalton Smith <daltzsmith@gmail.com>
@gerth2
Copy link
Contributor Author

gerth2 commented May 8, 2022

Thanks for all the early stuff - will work on incorporating it tonight and continuing to flesh out the actual wording on a lot of sections.

From the technical side:

How large is the JavaScript?

highcharts.js - ~300kb, already minified.
the new custom stuff - I'm expecting about 30kB when all is said and done. Currently ~1000 lines of js in one file, which really should get split up for comprehensibility.

@gerth2
Copy link
Contributor Author

gerth2 commented May 12, 2022

Ok poking the group for a design review on the .js side before I get too far.

55aecc0 should have most of the functional content required. It's close to being split out how I would like, with much less code duplication, and better class names. Not perfect yet but directionally where I want to go.

Additionally, conf.py has been updated to minify and concatenate all pid-tuning related javascript into a single file. However, as of that node, the files aren't getting concatenated in the right order, meaning the final output isn't actually valid javascript.

As far as getting include order right, the options I have researched:

  1. Custom RST Directive which handles some of the boilerplate HTML and imports the specified top-level js as a module, which in turn allows the JS itself to use include directives and self-define the class hierarchy. I like this because it feels the most-right, but seems the most intrusive to the whole frc-docs project in general.
  2. Implement some custom python classes to describe the javascript structure better and use that to get the order right. I don't like it because it seems like we're re-inventing the wheel with 1, and it causes the JS class hierarchy to get split between two spots (the js itself, and now some new python config).
  3. Prefix the js files with 0_, 1_, 2_ or whatever, then alphabetize the list prior to minimization and concatenization. Simple and non-intrusive, and keeps the class hierarchy all in the JS space. But smells hacky as all getout.
  4. Use Typescript. This feels "snazzy", but I have no idea what the implications of this would be. It would be the first time I've developed typescript.

Thoughts?

@gerth2
Copy link
Contributor Author

gerth2 commented May 13, 2022

FWIW - bb60a12ec3e implements option 3. It's functional, but hacky.

Oblarg added 5 commits August 13, 2022 01:15
…tch (#1)

* add turret tuning article, restructure some other stuff to match

* add section on when feedforward control is necessary

* update structure (not simulations) of flywheel article

* finish structural changes, propagate to vertical arm (minus some todos)

* update index, links, add intro to feedforward, general improvements

* try to restrict simulation to feedforward/feedback/both

* finish flywheel article revamp

* add click interaction to arm position demo

* fix x axis value

* Change vertical arm plant params to be reasonable

* finish vertical arm

* make vertical arm controller timing correct, record 10s

* color consistency, plot labeling

* add control effort to vertical arm visualization

* fix various build woopsies

* tune sizes

* Add vector representation to flywheel visualization

* add turret simulation

* improve recommended turret kd gain

* spelling and formatting
@Oblarg
Copy link
Contributor

Oblarg commented Aug 15, 2022

This seems good enough to merge now, to my eyes; it might need a couple of typographical changes per the Chief Delphi thread, but I don't think there are any major errors. I'm gonna go ahead and approve.

Oblarg
Oblarg previously approved these changes Aug 15, 2022
Copy link
Member

@Daltz333 Daltz333 left a comment

Choose a reason for hiding this comment

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

Additionally, I'd like to see most of the Wikipedia pages links/definitions recreated in the glossary section (for the tooltips), with the glossary definition linking to Wikipedia instead.

Copy link
Member

@Daltz333 Daltz333 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 I'm seeing unrelated changes to the conf.py and GitHub actions workflow.

@Daltz333 Daltz333 merged commit 9c5977f into wpilibsuite:main Aug 19, 2022
@TheTripleV
Copy link
Member

To only include the js on pages that need, this code could be wrapped in a directive that does it when it's processed.

gerth2 added a commit to gerth2/frc-docs that referenced this pull request Oct 7, 2022
* wip adding area for interactive walkthroughs

* WIP adding javascript to plot things

* more WIP getting interactive examples working

* changed to light colors, added button controls

* reorg and taking a few notes from the discord conversation

* reworked controls to be text entry. Added flywheel visualization.

* added time indication and other bug cleanup

* ball injection checkbox

* wip, more words

* Update source/docs/software/advanced-controls/introduction/approaches_to_ctrl_sys_design.rst

Co-authored-by: Dalton Smith <daltzsmith@gmail.com>

* Update source/docs/software/advanced-controls/introduction/approaches_to_ctrl_sys_design.rst

Co-authored-by: Dalton Smith <daltzsmith@gmail.com>

* kebab case, not snake case.

* WIP wording cleanups, missing wording in sections, section heading style

* Missed some section headings

* wip arm sim

* arm wip - getting closer!

* Graphical tweaks and adding cosin feed forward term

* wip javascript refactor

* wip automatically minifying a folder of js to a single included file

* renames and bugfixes

* visual apearance and link fixups

* more wording

* cleanup and methodlogy description

* Update source/docs/software/advanced-controls/introduction/introduction-to-pid.rst

Co-authored-by: Starlight220 <53231611+Starlight220@users.noreply.github.com>

* Update source/docs/software/advanced-controls/introduction/tuning-pid-controller.rst

Co-authored-by: Starlight220 <53231611+Starlight220@users.noreply.github.com>

* Update source/index.rst

Co-authored-by: Starlight220 <53231611+Starlight220@users.noreply.github.com>

* Update source/docs/software/advanced-controls/introduction/introduction-to-pid.rst

Co-authored-by: Starlight220 <53231611+Starlight220@users.noreply.github.com>

* Update source/docs/software/advanced-controls/introduction/tuning-pid-controller.rst

Co-authored-by: Starlight220 <53231611+Starlight220@users.noreply.github.com>

* Update source/docs/software/advanced-controls/introduction/tuning-pid-controller.rst

Co-authored-by: Starlight220 <53231611+Starlight220@users.noreply.github.com>

* Update source/docs/software/advanced-controls/introduction/pid-video.rst

Co-authored-by: Starlight220 <53231611+Starlight220@users.noreply.github.com>

* Update source/docs/software/advanced-controls/introduction/tuning-pid-controller.rst

Co-authored-by: Starlight220 <53231611+Starlight220@users.noreply.github.com>

* Update source/docs/software/advanced-controls/introduction/tuning-pid-controller.rst

Co-authored-by: Starlight220 <53231611+Starlight220@users.noreply.github.com>

* Update source/docs/software/advanced-controls/introduction/tuning-pid-controller.rst

Co-authored-by: Starlight220 <53231611+Starlight220@users.noreply.github.com>

* resolving spelling errors and hyphenated feed-forward

* resolved a bunch more conversations

* Update source/docs/software/advanced-controls/introduction/tuning-pid-controller.rst

Co-authored-by: Starlight220 <53231611+Starlight220@users.noreply.github.com>

* resolved the remainder of Yotam's questions

* wip covering some of oblarg's comments

* All the comments have been accounted for! Huzzah!

Now to make CI pass.

* lint fixups pt 1

* lint check fixups pt 2

* black window reformat

* attempt to fix sphinx build

* missed js renames which broke things.

* added ball to flywheel visualization

* adjusted PID range

* rewording and split voltage graph

* cleaned up multi-chart formatting, added more plant/controller terminology

* lint fixes

* Axis title fixups

* Reorganized tuning exercises to be by mechanism, not by control techinque

* fixing redirect and a bit of wording

* actually fixed redirects

* redirects append

* Delete black_windows.exe

Accidentally added, this should not be present.

* github comment fixup

* moved inputs to use text input (not number) to inhibit undesired validation behavior

* add turret tuning article, restructure some other stuff to match

* add section on when feedforward control is necessary

* update structure (not simulations) of flywheel article

* finish structural changes, propagate to vertical arm (minus some todos)

* update index, links, add intro to feedforward, general improvements

* try to restrict simulation to feedforward/feedback/both

* finish flywheel article revamp

* add click interaction to arm position demo

* fix x axis value

* Change vertical arm plant params to be reasonable

* finish vertical arm

* make vertical arm controller timing correct, record 10s

* color consistency, plot labeling

* add control effort to vertical arm visualization

* fix various build woopsies

* tune sizes

* Add vector representation to flywheel visualization

* add turret simulation

* improve recommended turret kd gain

* spelling and formatting

* [DRAFT] add turret tuning article, restructure some other stuff to match (#1)

* add turret tuning article, restructure some other stuff to match

* add section on when feedforward control is necessary

* update structure (not simulations) of flywheel article

* finish structural changes, propagate to vertical arm (minus some todos)

* update index, links, add intro to feedforward, general improvements

* try to restrict simulation to feedforward/feedback/both

* finish flywheel article revamp

* add click interaction to arm position demo

* fix x axis value

* Change vertical arm plant params to be reasonable

* finish vertical arm

* make vertical arm controller timing correct, record 10s

* color consistency, plot labeling

* add control effort to vertical arm visualization

* fix various build woopsies

* tune sizes

* Add vector representation to flywheel visualization

* add turret simulation

* improve recommended turret kd gain

* spelling and formatting

* Apply suggestions from code review

* File name cleanup and better comments/cleanup in conf.py

* unbork. I am bad at merging.

* finally fixed conf.py failing CI

Co-authored-by: Dalton Smith <daltzsmith@gmail.com>
Co-authored-by: Starlight220 <53231611+Starlight220@users.noreply.github.com>
Co-authored-by: Oblarg <emichaelbarnett@gmail.com>
@Starlight220 Starlight220 linked an issue Jan 23, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve PIDController Article

7 participants