bugst / go-serial

A cross-platform serial library for go-lang.
BSD 3-Clause "New" or "Revised" License
622 stars 191 forks source link

Windows: Set DTS via setCommState #35

Closed niondir closed 6 years ago

niondir commented 6 years ago

I found this comment:

func (port *windowsPort) SetRTS(rts bool) error {
    // It seems that there is a bug in the Windows VCP driver:
    // it doesn't send USB control message when the RTS bit is
    // changed, so the following code not always works with
    // USB-to-serial adapters.

But have the same problem for DTR, setting DTR does not work correctly when using escapeCommFunction.

Observed behavior: Connect -> RTS = true (low) DTR = true (low) OKAY SetDTR(false) -> RTS = true (low) DTR = false (heigh) OKAY SetRTS(false) -> RTS = false (heigh) DTR = true (low) ERROR: DTR toggled

This does not happen with the following fix, were one has better control over the state of the line:

func (port *windowsPort) SetDTR(dtr bool) error {
    /*
        var res bool
        if dtr {
            res = escapeCommFunction(port.handle, commFunctionSetDTR)
        } else {
            res = escapeCommFunction(port.handle, commFunctionClrDTR)
        }
        if !res {
            return &PortError{}
        }
    */

    params := &dcb{}
    if err := getCommState(port.handle, params); err != nil {
        return &PortError{causedBy: err}
    }
    params.Flags &= dcbDTRControlDisableMask
    if dtr {
        params.Flags |= dcbDTRControlEnable
    }
    if err := setCommState(port.handle, params); err != nil {
        return &PortError{causedBy: err}
    }

    return nil
}

Plus there is a typo in dcbRTSControlDisbaleMask -> Disable

niondir commented 6 years ago

I created a pull request. I hope it gets integrated.

Anyway, I have the feeling this lib is not maintained actively but seems to be the best alternative to deal with serial ports. I will continue to maintain a Fork on https://github.com/Lobaro/go-serial where even other features will get merged like the non-blocking operation. We deal a lot with embedded Hardware and serial ports and rely on a feature rich, cross OS serial lib.

cmaglie commented 6 years ago

hi @Niondir,

thanks for the PR, may I ask you to post an example that shows the problem solved by the PR? This will surely accelerate the integration.

Anyway, I have the feeling this lib is not maintained actively but seems to be the best alternative to deal with serial ports. I will continue to maintain a Fork on https://github.com/Lobaro/go-serial where even other features will get merged like the non-blocking operation.

I'm actively maintaining this library, together with other valuable contributors like @albenik or @jcw, but really a lot of people contributed. Sometimes I'm slow to merge new features or give feedback, that's true, unfortunately time is always a scarce resource. BTW I'm much more reactive for bugfixes.

This library is used inside the Arduino Create Agent and a regression may potentially affect hundreds of users, so my priority is about stability and cross-os compatibility that's why, usually, I need more time to carefully test new contributions that touches the core of the library (like for example #33).

I'm working also on a branch that adds an automated test-suite to check for regressions, that will surely give a boost in accepting new feature contributions.

niondir commented 6 years ago

Thanks for the quick response.

The example is what I wrote in "Observation" above and in the comment next to the fix.

The real world problem I'm solving is the following: DTR is used as the BOOT0 pin RTS is used as the RESET pin on a STM32l151CBA

To get into bootloader mode I need to set DTR=false while resetting with a raising RTS flank: RTS=true (low) -> RTS=false (high) This does not work with the current implementation since the DTR/BOOT pin is released when setting RTS, resulting in the chip starting into normal mode instead of bootloader mode.

niondir commented 6 years ago

In arduino-create-agent there is only one place where SetDTR(false) is called in upload.go touchSerialPortAt1200bps(...). As far as I can see and what I have tested the PR should not affect the behavior at all.

cmaglie commented 6 years ago

Fixed by #36, thank you!