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

Use /dev/ttyCAN0 instead of /dev/ttyACM9 #8

Merged
merged 2 commits into from
May 25, 2023

Conversation

seebq
Copy link
Member

@seebq seebq commented May 23, 2023

Because we were using /dev/ttyACM9 if you unplug and replug in enough tty devices, they eventually take that device name. It's reserved and the symlink apparently gets overridden or linux doesn't like you using that device name.

Just ask @AGummyBear how many times he had to manually unplug/replug devices (luckily he has per port power USB control and was able to do it via software).

Sadly, the software still requires one of these reserved names instead of a fancy unique udev name or by serial or some other more modern approach.

I tested this briefly by hardcoding the change in the udev rule on a machine and the emucd_64 binary worked:

sudo /usr/bin/emucd_64 -s6 /dev/ttyCAN0 can0 can1

$ ps auxww | grep emuc
root        3646  0.0  0.0   4004   132 ?        Ss   21:11   0:00 /usr/bin/emucd_64 -s6 /dev/ttyCAN0 can0 can1

This should minor version bump.

Would like to test installing this manual deb on a machine to make sure that it updates the udev rule appropriately.

I'm nervous the Debian installation won't allow overwriting this config file.

Copy link
Contributor

@kakaday22 kakaday22 left a comment

Choose a reason for hiding this comment

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

Curiosity, have you build this debian and install it on a test machine?

@seebq
Copy link
Member Author

seebq commented May 23, 2023 via email

@kakaday22
Copy link
Contributor

Yes, you can follow the README, it should still be accurate

@seebq
Copy link
Member Author

seebq commented May 24, 2023

Tested, installs cleanly and overrides udev and service.

@kakaday22
Copy link
Contributor

CC: @mfsurvilo

Copy link

@mfsurvilo mfsurvilo left a comment

Choose a reason for hiding this comment

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

Looks great, thanks guys

@seebq
Copy link
Member Author

seebq commented May 24, 2023

Public repo so being intentionally vague here, but last steps to merge:

  • Test on unit once @edavies64 gives go ahead
  • ask @AGummyBear how to trigger this (6 unplug/re-plugs) and make sure it can get past it
  • put on experimental server to deploy on our fleet

@kakaday22
Copy link
Contributor

I am noticing we still have some print statements to fix as well:

fprintf(stderr, "emucd_64 -v /dev/ttyACM0\n");
just to keep consistent

@seebq
Copy link
Member Author

seebq commented May 24, 2023

Sorry y'all, one more commit to update the start script and stop scripts with what Innodesk recommends. I don't like it one bit, using modprobe and rmmod inside a service file (scary!) but it's not only what their start and stop scripts do, but also it works. Tested with @edavies64 and we can now start and stop emuccan.service with this and CAN traffic resumes cleanly. Before, we could never get it to stop and start.

Now we can build a recovery behavior if canbus traffic fails, or if we get that dreaded:

kernel: emuc : bump : parse fail -1.

We can now stop and restart the emuccan.service.

@seebq
Copy link
Member Author

seebq commented May 25, 2023

I am noticing we still have some print statements to fix as well:

fprintf(stderr, "emucd_64 -v /dev/ttyACM0\n");

just to keep consistent

That's not our code.

@edavies64 also wants to upgrade this to latest we built this from, this is all their boilerplate.

I would rewrite a LOT of it if that's the case.

@edavies64
Copy link

I am noticing we still have some print statements to fix as well:

fprintf(stderr, "emucd_64 -v /dev/ttyACM0\n");

just to keep consistent

That's not our code.

@edavies64 also wants to upgrade this to latest we built this from, this is all their boilerplate.

I would rewrite a LOT of it if that's the case.

Talking with @seebq we're going to push on updating the driver to a later date.

@AGummyBear AGummyBear merged commit c343879 into main May 25, 2023
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.

5 participants