You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/01/28 21:31:28 UTC
[GitHub] [incubator-nuttx] davids5 opened a new pull request #5369: cdcacm:support returning c_cflag & speed via termios
davids5 opened a new pull request #5369:
URL: https://github.com/apache/incubator-nuttx/pull/5369
## Summary
Implementation was incomplete. This can now be
used to pass the linecodeing information to
a real serial port.
## Impact
Prior to this PR linecodeing was not complete when using TCGETS
## Testing
by inspection and using passthru to a real serial port.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #5369: cdcacm:support returning c_cflag & speed via termios
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #5369:
URL: https://github.com/apache/incubator-nuttx/pull/5369#discussion_r795215361
##########
File path: drivers/usbdev/cdcacm.c
##########
@@ -2342,7 +2342,11 @@ static int cdcuart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
termiosp->c_iflag = serdev->tc_iflag;
termiosp->c_oflag = serdev->tc_oflag;
termiosp->c_lflag = serdev->tc_lflag;
- termiosp->c_cflag = CS8;
+ termiosp->c_cflag =
+ ((priv->linecoding.parity != CDC_PARITY_NONE) ? PARENB : 0) |
Review comment:
@xiaoxiang781216 - OK - please merge it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5369: cdcacm:support returning c_cflag & speed via termios
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5369:
URL: https://github.com/apache/incubator-nuttx/pull/5369#discussion_r795208562
##########
File path: drivers/usbdev/cdcacm.c
##########
@@ -2342,7 +2342,11 @@ static int cdcuart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
termiosp->c_iflag = serdev->tc_iflag;
termiosp->c_oflag = serdev->tc_oflag;
termiosp->c_lflag = serdev->tc_lflag;
- termiosp->c_cflag = CS8;
+ termiosp->c_cflag =
+ ((priv->linecoding.parity != CDC_PARITY_NONE) ? PARENB : 0) |
Review comment:
@davids5 sorry, I misunderstand the PARENB meaning(enable parity check, not even parity check). I am fine with the change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #5369: cdcacm:support returning c_cflag & speed via termios
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #5369:
URL: https://github.com/apache/incubator-nuttx/pull/5369#discussion_r795202942
##########
File path: drivers/usbdev/cdcacm.c
##########
@@ -2342,7 +2342,11 @@ static int cdcuart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
termiosp->c_iflag = serdev->tc_iflag;
termiosp->c_oflag = serdev->tc_oflag;
termiosp->c_lflag = serdev->tc_lflag;
- termiosp->c_cflag = CS8;
+ termiosp->c_cflag =
+ ((priv->linecoding.parity != CDC_PARITY_NONE) ? PARENB : 0) |
Review comment:
> both CDC_PARITY_EVEN and CDC_PARITY_ODD will set If priv->linecoding.parity == CDC_PARITY_ODD, I don't think it's the right output.
I do not understand what you typed. May be you could rephrase it?
Here is how I understand this:
CDC settings
```
#define CDC_PARITY_NONE 0 /* No parity */
#define CDC_PARITY_ODD 1 /* Odd parity */
#define CDC_PARITY_EVEN 2 /* Even parity */
#define CDC_PARITY_MARK 3 /* Mark parity */
#define CDC_PARITY_SPACE 4 /* Space parity */
```
We do not support mark and space parity
Termios Settings:
```
#define PARENB (1 << 8) /* Bit 8: Parity enable */
#define PARODD (1 << 9) /* Bit 9: Odd parity, else even */
```
At the logical level:
PARENB (FALSE) when v = CDC_PARITY_NONE
PARENB (TRUE) when v = CDC_PARITY_ODD OR CDC_PARITY_EVEN (short circuited as NOT CDC_PARITY_NONE)
PARODD (FALSE) when v = CDC_PARITY_EVEN OR CDC_PARITY_NONE
PARODD (FALSE) when v = CDC_PARITY_ODD
|PARENB | PARODD | | |
| ---------|------------|-----------------| --------------------------|
| 0 | 0 | No Parity | CDC_PARITY_NONE |
| 1 | 1 | Odd Parity | CDC_PARITY_ODD |
| 1 | 0 | Even Parity | CDC_PARITY_EVEN |
@xiaoxiang781216 - The code matches the logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5369: cdcacm:support returning c_cflag & speed via termios
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5369:
URL: https://github.com/apache/incubator-nuttx/pull/5369#discussion_r795077712
##########
File path: drivers/usbdev/cdcacm.c
##########
@@ -2342,7 +2342,11 @@ static int cdcuart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
termiosp->c_iflag = serdev->tc_iflag;
termiosp->c_oflag = serdev->tc_oflag;
termiosp->c_lflag = serdev->tc_lflag;
- termiosp->c_cflag = CS8;
+ termiosp->c_cflag =
+ ((priv->linecoding.parity != CDC_PARITY_NONE) ? PARENB : 0) |
Review comment:
both CDC_PARITY_EVEN and CDC_PARITY_ODD will set If priv->linecoding.parity == CDC_PARITY_ODD, I don't think it's the right output.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5369: cdcacm:support returning c_cflag & speed via termios
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5369:
URL: https://github.com/apache/incubator-nuttx/pull/5369#discussion_r795206657
##########
File path: drivers/usbdev/cdcacm.c
##########
@@ -2342,7 +2342,11 @@ static int cdcuart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
termiosp->c_iflag = serdev->tc_iflag;
termiosp->c_oflag = serdev->tc_oflag;
termiosp->c_lflag = serdev->tc_lflag;
- termiosp->c_cflag = CS8;
+ termiosp->c_cflag =
+ ((priv->linecoding.parity != CDC_PARITY_NONE) ? PARENB : 0) |
Review comment:
Not sure about the logic, but as for me the readability of such code is bad. I suggest to split it into few lines:
```
termiosp->c_cflag = CS8;
if (priv->linecoding.parity != CDC_PARITY_NONE)
{
termiosp->c_cflag |= PARENB;
if (priv->linecoding.parity == CDC_PARITY_ODD)
{
termiosp->c_cflag |= PARODD;
}
}
if (priv->linecoding.stop == CDC_CHFMT_STOP2)
{
termiosp->c_cflag |= CSTOPB;
}
```
or a few ternary operators
```
termiosp->c_cflag = CS8;
termiosp->c_cflag |= (priv->linecoding.parity != CDC_PARITY_NONE) ? PARENB : 0;
termiosp->c_cflag |= (priv->linecoding.parity == CDC_PARITY_ODD) ? PARODD : 0;
termiosp->c_cflag |= (priv->linecoding.stop == CDC_CHFMT_STOP2) ? CSTOPB : 0;
```
##########
File path: drivers/usbdev/cdcacm.c
##########
@@ -2354,6 +2358,10 @@ static int cdcuart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
termiosp->c_cflag |= (priv->iflow) ? CRTS_IFLOW : 0;
Review comment:
```suggestion
termiosp->c_cflag |= priv->iflow ? CRTS_IFLOW : 0;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5369: cdcacm:support returning c_cflag & speed via termios
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5369:
URL: https://github.com/apache/incubator-nuttx/pull/5369#discussion_r795280102
##########
File path: drivers/usbdev/cdcacm.c
##########
@@ -2354,6 +2358,10 @@ static int cdcuart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
termiosp->c_cflag |= (priv->iflow) ? CRTS_IFLOW : 0;
Review comment:
Change is fine to me. Let's merge and do any other change as a follow up PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #5369: cdcacm:support returning c_cflag & speed via termios
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #5369:
URL: https://github.com/apache/incubator-nuttx/pull/5369#discussion_r795053787
##########
File path: drivers/usbdev/cdcacm.c
##########
@@ -2342,7 +2342,11 @@ static int cdcuart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
termiosp->c_iflag = serdev->tc_iflag;
termiosp->c_oflag = serdev->tc_oflag;
termiosp->c_lflag = serdev->tc_lflag;
- termiosp->c_cflag = CS8;
+ termiosp->c_cflag =
+ ((priv->linecoding.parity != CDC_PARITY_NONE) ? PARENB : 0) |
Review comment:
@xiaoxiang781216 Is it the double negative you do not like?
PARENB is enable. With your suggestion, would it be enabled on Odd parity?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5369: cdcacm:support returning c_cflag & speed via termios
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5369:
URL: https://github.com/apache/incubator-nuttx/pull/5369#discussion_r795208562
##########
File path: drivers/usbdev/cdcacm.c
##########
@@ -2342,7 +2342,11 @@ static int cdcuart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
termiosp->c_iflag = serdev->tc_iflag;
termiosp->c_oflag = serdev->tc_oflag;
termiosp->c_lflag = serdev->tc_lflag;
- termiosp->c_cflag = CS8;
+ termiosp->c_cflag =
+ ((priv->linecoding.parity != CDC_PARITY_NONE) ? PARENB : 0) |
Review comment:
@davids5 sorry, I misunderstand the PARENB meaning(enable parity check, not even parity check).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #5369: cdcacm:support returning c_cflag & speed via termios
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #5369:
URL: https://github.com/apache/incubator-nuttx/pull/5369#discussion_r795215555
##########
File path: drivers/usbdev/cdcacm.c
##########
@@ -2354,6 +2358,10 @@ static int cdcuart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
termiosp->c_cflag |= (priv->iflow) ? CRTS_IFLOW : 0;
Review comment:
@pkarashchenko - I here what your saying. This pattern is the same in other drivers. It could be a sperate PR to fix them all. This code has been tested and works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko merged pull request #5369: cdcacm:support returning c_cflag & speed via termios
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged pull request #5369:
URL: https://github.com/apache/incubator-nuttx/pull/5369
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5369: cdcacm:support returning c_cflag & speed via termios
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5369:
URL: https://github.com/apache/incubator-nuttx/pull/5369#discussion_r794991166
##########
File path: drivers/usbdev/cdcacm.c
##########
@@ -2342,7 +2342,11 @@ static int cdcuart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
termiosp->c_iflag = serdev->tc_iflag;
termiosp->c_oflag = serdev->tc_oflag;
termiosp->c_lflag = serdev->tc_lflag;
- termiosp->c_cflag = CS8;
+ termiosp->c_cflag =
+ ((priv->linecoding.parity != CDC_PARITY_NONE) ? PARENB : 0) |
Review comment:
```suggestion
((priv->linecoding.parity == CDC_PARITY_EVEN) ? PARENB : 0) |
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5369: cdcacm:support returning c_cflag & speed via termios
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5369:
URL: https://github.com/apache/incubator-nuttx/pull/5369#discussion_r795227668
##########
File path: drivers/usbdev/cdcacm.c
##########
@@ -2354,6 +2358,10 @@ static int cdcuart_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
termiosp->c_cflag |= (priv->iflow) ? CRTS_IFLOW : 0;
Review comment:
I am fine with the change now, @pkarashchenko do you have more concern?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org