Skip to content

diagnose: accept --id in any order - #177

Open
pradeepsuryad wants to merge 1 commit into
enactic:mainfrom
pradeepsuryad:diagnose-ids-any-order
Open

pradeepsuryad wants to merge 1 commit into
enactic:mainfrom
pradeepsuryad:diagnose-ids-any-order

Conversation

@pradeepsuryad

Copy link
Copy Markdown

Fixes #161.

This does both parts of the proposal in #161:

  1. run_diagnose() sorts send_ids and drops repeats right after parsing them. The report is then in ascending order, which section (2b) and explain() read as the daisy-chain order.
  2. Each row's id comes from openarm.get_arm().get_motors() rather than from the command line, so a row's label and its get_link_stats(i) come from the same place.

With today's library, (1) alone is enough; (2) keeps the labels right however the library orders its motors. --arm, and --id lists that are already ascending, give the same report as before. This doesn't depend on #176.

While testing, I found that --id 1,1 on main does more than leave an empty row. It prints the empty row in (1) and (2), then stops at (2b) with exit status 1:

✗ Diagnose Error: vector::_M_range_check: __n (which is 1) >= this->size() (which is 1)

The repeated id gets a second row, but the library registered only one motor, so get_link_stats(1) is out of range.

Test

No motors needed. On vcan0 (GitHub Actions, Ubuntu 22.04 and 24.04), a small script answers as motor 0x01. It replies to enable, disable and refresh with a state frame on 0x11. Nothing answers as 0x08, as if the chain were broken between them. diagnose runs on main and on this branch:

Command main This branch
--id 8,1 --explain 0x01's replies are counted on the 0x08 row and 0x08's silence on the 0x01 row; (2b) and --explain say the axes answer up to 0x08 and stop from 0x01 0x01 answers and 0x08 is silent; (2b) and --explain put the break between 0x01 and 0x08
--id 1,8 --explain correct same as main
--id 1,1 a second, empty row, then the error above one row, exit status 0

Run: https://github.com/pradeepsuryad/openarm_can/actions/runs/42080497698

The fake motor and the check script are on a separate branch of my fork. I can add them to the repository as a test if you'd like one.

run_diagnose() took each row's id from the command line and its
statistics from get_link_stats(i), so the two only matched when the ids
were given in the order the library holds the motors in. With --id 8,1
the rows, the (2b) boundary and the --explain advice named the wrong
motors, and --id 1,1 added a second row for a motor registered once.

Sort the ids and drop repeats as soon as they are parsed: the report is
then in ascending order, which section (2b) and explain() read as the
daisy-chain order. Take each row's id from the motors the library
registered, so a row's label and its statistics come from one place.

Fixes enactic#161.
@packit-as-a-service

Copy link
Copy Markdown

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo dnf install -y 'dnf*-command(copr)'
  • dnf copr enable packit/enactic-openarm_can-177
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

This branch has not been deployed

No deployments
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.

diagnose: accept --id in any order

1 participant