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

Electrical Wiring: Pinch Valve Harness #1336

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

inceptionev
Copy link
Contributor

@inceptionev inceptionev commented Apr 22, 2023

Description

This PR creates the definition and instructions for making the pinch valve harness.

Closes #1284

Self-review checklist:

  • Self-review: looked through the Files changed tab, browsed repository in branch

  • Documentation updated - reflects changes in code, electrical or mechanical design

  • Documentation and graphics follow the documentation style guide

  • New content is linked, easily discoverable, does not require too many clicks

  • Follows other relevant parts of contributor wiki

  • PR has a descriptive name

  • Tagged relevant reviewers

  • Follows our mechanical engineering standards

  • Everything is metric and ISO where possible

  • All features and constraints are named

  • Imported parts are well-named, with source URL and iProperties

  • Screenshots and/or assembly photos updated

@codecov
Copy link

codecov bot commented Apr 22, 2023

Codecov Report

Merging #1336 (b448d3b) into master (8154de8) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1336   +/-   ##
=======================================
  Coverage   57.53%   57.53%           
=======================================
  Files         102      102           
  Lines        3471     3471           
  Branches      321      321           
=======================================
  Hits         1997     1997           
  Misses       1474     1474           
Flag Coverage Δ
common 95.90% <ø> (ø)
controller 57.37% <ø> (ø)
gui 29.42% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@martukas
Copy link
Member

When building this locally, I get the following errors related to the wiring docs:

docs/wiring/oxygen_sensor.rst:4: WARNING: duplicate label oxygen sensor harness, other instance in docs/wiring/oxygen_sensor.rst
docs/wiring/pinch_valve.rst:4: WARNING: duplicate label pinch valve harness, other instance in docs/wiring/pinch_valve.rst

There is the .. _Pinch Valve Harness which is not rendered but only used for reference links. The section name that looks like this:

Pinch Valve Harness
======================

constitutes the document name and is also taken up by sphinx as a xref identifier. If the latter is enough thee is no need for the former. See blower for a case where they are different, and in that case it probably makes sense.

And also

docs/wiring/blower_TK_BA7050H12B.rst:12: WARNING: undefined label: included-with-blower
docs/wiring/pinch_valve.rst:12: WARNING: undefined label: included-with-motor

I understand the issue, but not sure how we would want this to ideally function? Should it still link to the blower/motor item in the BOM somehow? If not, should they at least render nicer? What would do we want them to say on the table? If we expect to have similar but not identical opt-out/dummy part numbers, how should they be identified by the script as not needing a link? Perhaps you could create a ticket with a feature request for improvement to the docs script? If some answers to the above questions are not obvious, we could have a small discussion to hone in on what is needed.

@martukas
Copy link
Member

Perhaps like we did for the power harness, we could also include an alternate pre-crimped wires?
In this case I got these:
https://www.digikey.com/short/94m2z0bw

@martukas
Copy link
Member

martukas commented Apr 23, 2023

The 1m wire length is empirically what comes with the stepper. Now examining the enclocure it looks like at the worst case (valve at opposite corner from the connector, taking some non-straight-line path) it should be no more than 70cm, probably less when we finalize it. We could also create a ticket to make a final revision of harness lengths prior to final release of 0.4.

@martukas
Copy link
Member

Looks like 4mm diameter shrink tubing might be more appropriate here, unless we are talking solder joints to pre-crimped tails, in which case yes 5mm.

@martukas
Copy link
Member

And I used maybe 6-7 pieces of shrink tubing. The stepper wires are pretty twisty and capricious.

Copy link
Member

@martukas martukas 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 managed to make it. Notes from the process are provided above as comments on the PR. I don't think the changes I am requesting are very significant and you are welcome to evade them or postpone them via new tickets, for example.
Anyway, the only aspect of this that I cannot verify is whether the wire mappings are correct without a working HAL. The STMduino code rotates the stepper about 180 degrees clockwise at a slow pace, so that is tentatively something.

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.

Wire Harness: Pinch Valve
2 participants