Re: [PATCH v4] usbserial: cp210x - icount support for parity error checking

[Johan Hovold <johan@kernel.org>]

On Mon, Jul 06, 2020 at 01:47:50PM +0200, Jerry wrote:
> Johan Hovold wrote on 7/3/20 5:01 PM:
> > On Fri, Jul 03, 2020 at 09:45:39AM +0200, Johan Hovold wrote:
> > Would you mind giving the below a try and see how that works in your
> > setup?
> >
> > With this patch you'd be able to use PARMRK to be notified of any parity
> > errors, but I also wired up TIOCGICOUNT if you prefer to use that.
> >
> > Also, could try and see if your device detects breaks properly? Mine
> > just return a NUL char.

> I tried your patch. It detects the parity error correctly for my 
> application. Unfortunately I'm not able currently to verify a break 
> reception due to holiday, I'll probably try it next week when back at work 
> (I need to recompile the device firmware).

Cool, thanks.

I noticed that I can get comm-status to report a break condition when
event-insertion mode is disabled, but it just results in a NUL char in
event mode (the error flag isn't set then).

Perhaps buggy firmware, unless there's some other command for masking
events. Haven't quite understood how the EVENTMASK requests are supposed
to be used. Replacing the break char (SET_CHAR) didn't help at least.

> An interesting thing that your patch don't report any overrun. I'm not able 
> to create a real overrun (any idea?) but it doesn't report any fake overrun 
> compared to GET_COMM_STATUS.

Yeah, I noticed that too, although I had a feeling the fake overrun
didn't even always show up when sending while closed here either. 

Not sure how best to trigger an overrun since we rely on the read urb to
report it. Perhaps pausing the read urbs, filling up the device fifo and
then submitting the urbs again could work? Would need to patch the
driver quite a bit for that though.

> The question could be a value of CP210X_ESCCHAR. You selected 0xFF and this 
> value can be quite common in received data because an erased flash memory 
> is full of 0xFF. When I read an empty memory it means double USB bandwidth 
> so for example 0xFE seems be better... but I understand that it depend on 
> application and it is hard to globally select this value.

Good point! I think we should definitely avoid 0xff.

> I'll do some more tests but your solution seems be fully acceptable for my 
> needs. For me TIOCGICOUNT is enough (I just resend request when an error 
> detected) but for other situation it would be very nice to know exactly 
> which byte was malformed through PARMRK.

That's good to hear. I'll respin the generic (event-based) solution
then.

Thanks,
Johan