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.

Sign up for GitHub

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

Jump to bottom

Fix 1200-bps touch DTR handling (Windows) #2234

Merged
merged 1 commit into from
Jun 30, 2023
Merged

Fix 1200-bps touch DTR handling (Windows) #2234

merged 1 commit into from
Jun 30, 2023

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Jun 30, 2023 •
edited
Loading

Please check if the PR fulfills these requirements

See how to contribute

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • configuration.schema.json updated if new parameters are added.

What kind of change does this PR introduce?

This PR disables the step that sets DTR low on the 1200-bps touch subroutine.

This is the reason why it was originally introduced: arduino/Arduino@a6909bd

Fix auto-reset on Leonardo-derived boards from Linux hosts
Also renamed the touchPort() function, as it's now unambiguously
single-purpose.

The 1200bps reset from Linux hosts wasn't working with these newer
JSSC-based versions. Adding a step which explicitly sets DTR low (via a
TIOCMSET ioctl clearing DTR) fixes this.

I'm fairly sure the reason why this worked on older Arduino with librxtx
and not with jssc is that librxtx appears to keep HUPCL in the termio
flags, but jssc appears to remove it. If HUPCL ("hangup on close") is
set, it causes DTR to be explicitly pulled low on close.

Why we are removing it now?

  • Windows does preserve the state of the RTS/DTR bits on the next opening of the serial port.
  • The serial library used in the Arduino IDE 1.8.x has a bug when trying to set DTR=false, on successive opening of the port the DTR line is set back high by the USB serial driver. This works differently from the serial library we use in the Arduino CLI, which sets DTR=false for good and this setting is preserved on the next opening of the port.
  • Having the serial port left in a state with DTR=false may cause problems with tools uploading later.

It may probably be applied to all OS but, for now, it will be disabled only for Windows to reduce the testing surface.

What is the current behavior?

Upload may fail on some boards (arduino/ArduinoCore-renesas#10).

What is the new behavior?

Uploads should be successful...

Does this PR introduce a breaking change, and is titled accordingly?

No, in theory. A lot of testing is required to be sure.

Other information

@cmaglie
Disable DTR clearing on 1200-bps touch (only on Windows)
31d9c34 
The reason why it was originally introduced:
arduino/Arduino@a6909bd

Why we are removing it now?
* Windows does preserve the state of the RTS/DTR bits on successive
  opening of the serial port.
* The serial library used in the Arduino IDE 1.8.x has a bug when trying
  to set DTR=false, on successive opening of the port the DTR line is
  set back high by the USB serial driver. This works differently from
  the serial library we use in the Arduino CLI, that sets DTR=false for
  good and this change is preserved on the successive opening of the
  port.
* Having the serial port left in a state with DTR=false may cause
  problems to tools uploading later.

It may probably completely removed, but for now, to reduce the testing
surface, it will be disabled only for Windows.
@cmaglie cmaglie self-assigned this Jun 30, 2023
@kittaakos kittaakos mentioned this pull request Jun 30, 2023
Closed
4 tasks
@cmaglie
Copy link
Member Author

cmaglie commented Jun 30, 2023

Failing checks are OK ✅. They are happening because this PR is against an old release branch.
It will be eventually cherry-picked on master.

@cmaglie cmaglie added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project priority: high Resolution is a high priority criticality: highest Of highest impact labels Jun 30, 2023
@kittaakos kittaakos mentioned this pull request Jun 30, 2023
Merged
4 tasks
@cmaglie cmaglie merged commit b08dbd5 into arduino:0.32.x Jun 30, 2023
@cmaglie cmaglie deleted the fix_touch_dtr_handling branch June 30, 2023 11:39
kittaakos pushed a commit to arduino/arduino-ide that referenced this pull request Jun 30, 2023
chore(cli): Bumped the CLI version to 0.32.3
a03d1be 
Ref: arduino/arduino-cli#2234
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
@kittaakos kittaakos mentioned this pull request Jun 30, 2023
Merged
4 tasks
kittaakos pushed a commit to arduino/arduino-ide that referenced this pull request Jun 30, 2023
@kittaakos
chore(cli): Bumped the CLI version to 0.32.3
f3b99b0 
Ref: arduino/arduino-cli#2234
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
kittaakos pushed a commit to arduino/arduino-ide that referenced this pull request Jun 30, 2023
chore(cli): Bumped the CLI version to 0.32.3
c318223 
Ref: arduino/arduino-cli#2234
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
cmaglie added a commit that referenced this pull request Jun 30, 2023
@cmaglie
Disable DTR clearing on 1200-bps touch (only on Windows) (#2234)
347bfeb 
The reason why it was originally introduced:
arduino/Arduino@a6909bd

Why we are removing it now?
* Windows does preserve the state of the RTS/DTR bits on successive
  opening of the serial port.
* The serial library used in the Arduino IDE 1.8.x has a bug when trying
  to set DTR=false, on successive opening of the port the DTR line is
  set back high by the USB serial driver. This works differently from
  the serial library we use in the Arduino CLI, that sets DTR=false for
  good and this change is preserved on the successive opening of the
  port.
* Having the serial port left in a state with DTR=false may cause
  problems to tools uploading later.

It may probably completely removed, but for now, to reduce the testing
surface, it will be disabled only for Windows.
cmaglie added a commit that referenced this pull request Jun 30, 2023
@cmaglie
Disable DTR clearing on 1200-bps touch (only on Windows) (#2234)
fe6d749 
The reason why it was originally introduced:
arduino/Arduino@a6909bd

Why we are removing it now?
* Windows does preserve the state of the RTS/DTR bits on successive
  opening of the serial port.
* The serial library used in the Arduino IDE 1.8.x has a bug when trying
  to set DTR=false, on successive opening of the port the DTR line is
  set back high by the USB serial driver. This works differently from
  the serial library we use in the Arduino CLI, that sets DTR=false for
  good and this change is preserved on the successive opening of the
  port.
* Having the serial port left in a state with DTR=false may cause
  problems to tools uploading later.

It may probably completely removed, but for now, to reduce the testing
surface, it will be disabled only for Windows.
@per1234 per1234 added the os: windows Specific to Windows operating system label Jun 30, 2023
@per1234 per1234 mentioned this pull request Jul 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@umbynos umbynos umbynos approved these changes

Assignees

@cmaglie cmaglie

Labels
criticality: highest Of highest impact os: windows Specific to Windows operating system priority: high Resolution is a high priority topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cmaglie @umbynos @per1234