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

Improve target monitor and add target stty_params. #532

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

Conversation

doronbehar
Copy link
Contributor

Use less as the default MONITOR_CMD. Change comment explanation about
# Serial Monitor.

Add stty_params target.

Make stty_params run a command according to Arch Linux's WiKi article on
Arduino #stty. Make monitor target depend on stty_params.

@doronbehar doronbehar changed the title Imorove target monitor and add target stty_params. Improve target monitor and add target stty_params. Jan 29, 2018
@sudar
Copy link
Owner

sudar commented Sep 30, 2018

@doronbehar

Thanks for the PR.

Unfortunately I didn't get to it on time and it has resulted in some merge conflicts.

Can you please update your branch and fix the merge conflict?

@doronbehar
Copy link
Contributor Author

Rebased against master.

@sej7278
Copy link
Collaborator

sej7278 commented Sep 30, 2018

i'm also not sure what advantage less has over screen?

@wingunder
Copy link
Contributor

i'm also not sure what advantage less has over screen?

I totally agree.

I have no problem to have less and cat as alternatives, but screen is currently the default and I vote for it to remain like that.

@doronbehar
Copy link
Contributor Author

Personally, as for setting less as default, my opinion is that we should put defaults which are installed by default on almost every system. I can revert that if you really disagree with me..

@doronbehar
Copy link
Contributor Author

In addition to less being installed on almost every machine by default, when using it just as a reader, one can send multiple characters commands to the port (in another terminal), as exampled in the echo command here: https://wiki.archlinux.org/index.php/Arduino#stty

@sudar
Copy link
Owner

sudar commented Oct 7, 2018

@doronbehar

Changing the default monitor program will also break it for existing users. So I think it would be better to leave the default monitor program. Also if someone wants to change they can easily override the variable.

If you can remove the changes you made to set less as the default program and re-submit the PR, we could revisit it again.

@doronbehar
Copy link
Contributor Author

Pushed a revert for your request.

@sudar
Copy link
Owner

sudar commented Oct 10, 2018

@sej7278

What are your thoughts on the new stty_params target that is added?

Make it run a command according to arch wiki article on Arduino #stty.
Make `monitor` target depend on `stty_params`.
Use `less` as the default `MONITOR_CMD`.
Change comment explenation about `# Serial Monitor`.
Add support for tail as `MONITOR_CMD`.
@doronbehar doronbehar force-pushed the add-target-stty_params branch from 3652a82 to e76be67 Compare October 11, 2018 06:49
@doronbehar
Copy link
Contributor Author

Rebased again against master.

@sudar and @sej7278, I don't know how it goes on your machines but I use Arch Linux and when I run make monitor I sometimes get a bunch of gibberish characters and sometimes I get no output at all. With the stty_params target, I get a clear output just like I programmed.

@sej7278
Copy link
Collaborator

sej7278 commented Oct 11, 2018

the arch arduino package is all sorts of broken so that doesn't surprise me, i've never had a problem on debian/fedora/osx. what's in your ~/.screenrc?

i don't like that stty_params is going to be called in all cases when running "make monitor" even for users without the problem, which could potentially introduce problems for them.

it seems this is changing arduino-mk to work around some sort of archlinux bug.

@doronbehar
Copy link
Contributor Author

Thanks for your comment @sej7278, I might ask the Arch Linux community about the subject and inspect the way the arduino packages are built on other distributions.

I must say though, that I hardly believe there is an automated process that an OS can be configured to set these parameters by default whenever a new Arduino device is connected (perhaps using udev?), since there is an stty parameter defining the frequency of the communication between the two devices which is obviously different for every project..

Thinking about the frequency parameter, as a 1st idea, I was wondering, perhaps there is a default value being set by your OS's configuration that is identical to the one you are using in all of your Arduino projects? And that is why you never need to run any stty command on your machines?

Please leave this PR open for a while, I'll be glad to update you when you or my self will learn something new..

@sej7278
Copy link
Collaborator

sej7278 commented Oct 11, 2018

i'd like to figure out what's going wrong with your setup so we'll keep the PR open to monitor (!) your updates.

when you mention setting frequency for different devices, you can either set $MONITOR_BAUDRATE, or it'll read it from Serial.begin() or defaults to 9600 8 N 1

that's not the sort of thing set in udev, that's traditionally for setting permissions for usbasp's or stlink/maple devices (generally add yourself to the dialout group and you're good to go).

@doronbehar
Copy link
Contributor Author

I have made further tests and it seems the stty command is needed only when using cat, tail or less and not screen. I've found many suggestions around the internet for invoking a stty command:

I think making the target stty_params a dependency of the target monitor should improve our safety anyway since printing unprintable characters in a terminal is known to be dangerous. I'm willing to add more commits that will make the monitor target run the stty_params target only when a MONITOR_CMD other then screen is used but I don't see any advantage in this.

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.

4 participants