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

feature: VID2805 Dual Stepper Element #137

Merged
merged 8 commits into from
Sep 30, 2022

Conversation

bonnyr
Copy link
Contributor

@bonnyr bonnyr commented Sep 8, 2022

Adding a new element to represent a common dual stepper - the VID28-05

The part looks like this:
image

The component looks like this (top view):
image

and with shorter hands:
image

The part has two sets of the typical stepper pins (A-/A+/B-/B+)

One note is that I am not sure whether the pinInfo coords match the position in the drawing or how to make sure they do.
Another note is that on storybook, I am not sure how to create two group of controls, one for each stepper hand and be able to use these controls when initializing a class representing each of the hands

@urish
Copy link
Collaborator

urish commented Sep 9, 2022

Error message from Netlify:

4:11:04 AM: src/vid2805-dual-stepper-element.ts(88,27): error TS2550: Property 'replaceAll' does not exist on type 'string'. Do you need to change your target library? Try changing the 'lib' compiler option to 'es2021' or later.
4:11:04 AM: src/vid2805-dual-stepper-element.ts(89,27): error TS2550: Property 'replaceAll' does not exist on type 'string'. Do you need to change your target library? Try changing the 'lib' compiler option to 'es2021' or later.

You can probably see the same errors if you run npm run build locally.

replaceAll is newer JavaScript function, and isn't supported yet by all browsers. An alternative would be to use someString.split("search").join("replace")

@bonnyr
Copy link
Contributor Author

bonnyr commented Sep 10, 2022

Thanks for the clarification @urish

This now builds correctly.
There's the issues of:

  1. I am not sure whether the pinInfo coords match the position in the drawing or how to make sure they do.
  2. In storybook, I am not sure how to create two group of controls, one for each stepper hand and be able to use these controls when initializing a class representing each of the hands - currently the controls do not really translate into a change in the selected template

Any help here would be appreciated

@urish
Copy link
Collaborator

urish commented Sep 10, 2022

I am not sure whether the pinInfo coords match the position in the drawing or how to make sure they do.

Look at the Debugging Pin Info section of the Contributing guide to learn how to test it

In storybook, I am not sure how to create two group of controls, one for each stepper hand and be able to use these controls when initializing a class representing each of the hands - currently the controls do not really translate into a change in the selected template

I'm not sure if storybook supports groups of controls - you can take a quick look at their docs / examples and see if they do. Otherwise, you can probably use a prefix to separate the controls (e.g. hand1Something and hand2Something). Once I have an opportunity to look at the your code (probably some time next week), I will have more context to help.

@bonnyr
Copy link
Contributor Author

bonnyr commented Sep 13, 2022

@urish pinInfo stumps me...

The SVG for the element is using specific size in mm, the pins are drawn using relative path operations and are scaled by mmToPix :

<svg id="Layer_2" xmlns="http://www.w3.org/2000/svg" width=70mm height=70mm viewBox="0 0 180 180">
    <defs>
       ...
    </defs>
    <g id="Layer_1-2" transform="translate(0,${trY})">
        <g id="scaled_body">
          <g id="pins" transform="scale(${mmToPix})>
          <path id="pin-1" class="cls-3" d="m 58 0 v 5 c 0 2 2 2 2 0 v -5 z" />
          <path id="pin-2" class="cls-3" d="m 65 0 v 5 c 0 2 2 2 2 0 v -5 z" />

When returning pin info with the x coordinate the same as that specified in the path, the debug view shows the dots too close together:

  get pinInfo(): ElementPin[] {
    const { x, y, innerOff, outerOff, trY} = this.coords();

    return [
    { name: 'A1+', y: trY, x: 58 * 1, number: 1, signals: [] },
    { name: 'A1-', y: trY, x: 65  * 1, number: 2, signals: [] },
    { name: 'B1+', y: trY, x: 72  * 1, number: 3, signals: [] },

image

When scaling the information in pinInfo by mmToPix the dots are too spaced too wide apart:

 get pinInfo(): ElementPin[] {
    const { x, y, innerOff, outerOff, trY} = this.coords();

    return [
    { name: 'A1+', y: trY, x: 58 * mmToPix, number: 1, signals: [] },
    { name: 'A1-', y: trY, x: 65  * mmToPix, number: 2, signals: [] },

image

I've pushed the code that depicts that - any help would be appreciated.

@bonnyr
Copy link
Contributor Author

bonnyr commented Sep 19, 2022

@urish - managed to get the component and pins aligned and correct size - are you able to look at the component and accept/reject?

@urish urish self-requested a review September 20, 2022 08:15
Copy link
Collaborator

@urish urish left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

I left a bunch of comments. The most important point is regarding the innerHand / outerHand attribute. The other points are mostly regarding naming convention, removing debug prints, improving code readability, etc.

One more important point is the name of the element. I looked at the datasheet, and it seems like the real part is called vid28-05 and not vid2805, even though the datasheet describes it as vid28-xx.

I suggest to go with wokwi-vid28-dual-stepper. When you google "VID28" it comes up with images of this specific stepper motor, so the term "vid28" seems to be specific enough (and I prefer shorter names where possible).

src/vid2805-dual-stepper-element.ts Outdated Show resolved Hide resolved
src/vid2805-dual-stepper-element.ts Outdated Show resolved Hide resolved
src/vid2805-dual-stepper-element.ts Outdated Show resolved Hide resolved
src/vid2805-dual-stepper-element.ts Outdated Show resolved Hide resolved
src/vid2805-dual-stepper-element.ts Outdated Show resolved Hide resolved
src/vid2805-dual-stepper-element.ts Outdated Show resolved Hide resolved
src/vid2805-dual-stepper-element.ts Outdated Show resolved Hide resolved
src/vid2805-dual-stepper-element.stories.ts Outdated Show resolved Hide resolved
src/vid2805-dual-stepper-element.ts Outdated Show resolved Hide resolved
src/vid2805-dual-stepper-element.ts Outdated Show resolved Hide resolved
@bonnyr bonnyr force-pushed the add-vid2805-dual-stepper branch from ded0fde to 81799e2 Compare September 22, 2022 12:37
1. renamed component name tyo be more generic
2. remove debug stateents
3. added ornate hands
4. refactored hand path template expansion to be more TS/JS friendly
5. removed use of composite Hand classes as properties
@bonnyr bonnyr force-pushed the add-vid2805-dual-stepper branch from 81799e2 to 931a77b Compare September 22, 2022 12:38
@urish
Copy link
Collaborator

urish commented Sep 22, 2022

Thanks! "ornate" is cool. I'll hope to go over the actual changes soon!

@urish
Copy link
Collaborator

urish commented Sep 28, 2022

Thanks again for making the changes!

Looking at it, here are some issues which will affect the usability on Wokwi:

  1. The bounding SVG element is too big. We need it to be as tight as possible, as Wokwi uses this when drawing the selection rectangle around the element.
    image

  2. The element and pins moves when you change the arm length. Let's horizontally center the element within the SVG, so it stays in the same places (and the pins too), regardless of the hand length.

If we made the maximum hand length shorter (e.g. 50 instead of 70), we'd need to leave less padding around the element. I'm not super concerned about the current amount of padding, so if there's a good reason to keep it at 70, that's fine too.

@bonnyr
Copy link
Contributor Author

bonnyr commented Sep 28, 2022

The bounding SVG element is too big. We need it to be as tight as possible, as Wokwi uses this when drawing the selection rectangle around the element.

and

The element and pins moves when you change the arm length. Let's horizontally center the element within the SVG, so it stays in the same places (and the pins too), regardless of the hand length.

I had to make this as large as it is to account for the longer arms - they extend beyond the base.
The use case for making this element this large is when placing a number of these vertically - the hands then forms circles that are closer together.

If this is not compatible with the way Wokwi works, I am happy with your suggestion of reducing the maximum size.
This will also solve the movement issue since the arms will be confined to the bounding box of the element itself and we can make the element as tight as possible.

Thoughts?

@urish
Copy link
Collaborator

urish commented Sep 29, 2022

I had to make this as large as it is to account for the longer arms - they extend beyond the base.

Right now, there's extra margin on the right side - the photo that I took in the previous has the largest arm size.

I see two ways to go about this:

  1. Reducing the SVG size so it has just enough padding to fit tightly when the arms are in their largest size (perhaps reducing the max arm length a bit, so we need less padding)
  2. Introducing some mechanism to tell Wokwi about the true bounding box of the element (similar to what we have in pinInfo). That will probably also be useful for Robot Arm Element - Adds robot arm to storyboard. #119 if it ever lands.

The first option will be faster, as the second one needs some changes on the Wokwi side as well.

In any case, the pin positions / motor body should stay static even when you change the arm length.

I hope this all makes sense to you.

…lement size as much as possible. stopped moving the element based on hand size as requested.
@bonnyr
Copy link
Contributor Author

bonnyr commented Sep 30, 2022

@urish - another update, addressing the comments I believe

@urish urish merged commit faee44c into wokwi:master Sep 30, 2022
@urish
Copy link
Collaborator

urish commented Sep 30, 2022

Thank you!

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.

3 participants