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

apps/testcase/systemio/itc: add new UART TCs #5050

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

apps/testcase/systemio/itc: add new UART TCs #5050

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
700a0e3
framework/src/iotbus: add new APIs in iotbus to test new UART TCs
deepaksrma Sep 6, 2021
23a21a7
apps/testcase/systemio/itc: add new UART TCs
deepaksrma Sep 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
framework/src/iotbus: add new APIs in iotbus to test new UART TCs 
- add new APIs in iotbus to test UART TCs, APIs are:
	iotbus_uart_rxavailable()
	iotbus_uart_txready()
	iotbus_uart_txempty()

- add new TCs to test UART rxavailable, txready and txempty APIs.

Signed-off-by: Deepak Sharma <[email protected]>
  • Loading branch information
@deepaksrma
deepaksrma committed Sep 7, 2021
commit 700a0e394addbec822bbaa8c1cdbc8197318c0d3
66 changes: 66 additions & 0 deletions framework/src/iotbus/iotbus_uart.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,72 @@ int iotbus_uart_set_flowcontrol(iotbus_uart_context_h hnd, int xonxoff, int rtsc
}
#endif

int iotbus_uart_rxavailable(iotbus_uart_context_h hnd)
{
int ret;
int fd;
struct _iotbus_uart_s *handle;

if (!hnd || !hnd->handle) {
return IOTBUS_ERROR_INVALID_PARAMETER;
}

handle = (struct _iotbus_uart_s *)hnd->handle;
fd = handle->fd;

ret = ioctl(fd, TIOCS_AVAIL, 0);
if (ret != true) {
ibdbg("ioctl failed: receive fifo is empty\n");
return IOTBUS_ERROR_UNKNOWN;
}

return IOTBUS_ERROR_NONE;
}

int iotbus_uart_txready(iotbus_uart_context_h hnd)
{
int ret;
int fd;
struct _iotbus_uart_s *handle;

if (!hnd || !hnd->handle) {
return IOTBUS_ERROR_INVALID_PARAMETER;
}

handle = (struct _iotbus_uart_s *)hnd->handle;
fd = handle->fd;

ret = ioctl(fd, TIOCS_READY, 0);
if (ret != true) {
ibdbg("ioctl failed: transmit fifo is full\n");
return IOTBUS_ERROR_UNKNOWN;
}

return IOTBUS_ERROR_NONE;
}

int iotbus_uart_txempty(iotbus_uart_context_h hnd)
{
int ret;
int fd;
struct _iotbus_uart_s *handle;

if (!hnd || !hnd->handle) {
return IOTBUS_ERROR_INVALID_PARAMETER;
}

handle = (struct _iotbus_uart_s *)hnd->handle;
fd = handle->fd;

ret = ioctl(fd, TIOCS_EMPTY, 0);
if (ret != true) {
ibdbg("ioctl failed: transmit fifo is not empty\n");
return IOTBUS_ERROR_UNKNOWN;
}

return IOTBUS_ERROR_NONE;
}

int iotbus_uart_read(iotbus_uart_context_h hnd, char *buf, unsigned int length)
{
int fd;
Expand Down
27 changes: 27 additions & 0 deletions os/arch/arm/src/amebad/amebad_serial.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,8 @@
*/

static serial_t* sdrv[MAX_UART_INDEX + 1] = {NULL, NULL, NULL, NULL, NULL}; //uart 0~3, uart2 is configured as log uart
static bool g_tc_rxavailable;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment explaining the meaning of this variable.

static bool g_tc_rxavailable_res; // used to hold the result of rxavailable TC
Comment on lines +178 to +179
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree adding codes here for tc.

Copy link
Contributor Author

@deepaksrma deepaksrma Sep 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check my comment below and let me know what you think?
#5050 (comment)


struct rtl8721d_up_dev_s {
uint8_t parity; /* 0=none, 1=odd, 2=even */
Expand Down Expand Up @@ -571,6 +573,22 @@ static int rtl8721d_up_ioctl(FAR struct uart_dev_s *dev, int cmd, unsigned long
serial_control_loopback(sdrv[uart_index_get(priv->tx)], *(bool *)arg);
break;

case TIOCS_AVAIL:
g_tc_rxavailable = 1;
g_tc_rxavailable_res = 0;
rtl8721d_up_send(dev, 'a');
sleep(1);
ret = g_tc_rxavailable_res;
Comment on lines +577 to +581
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what you want to do. Could you leave comment to explain?

Copy link
Contributor Author

@deepaksrma deepaksrma Sep 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for rxavailable TC, my approach is:

  • rxavailable returns true when there is data in rx fifo and when data comes on the tx line in loopback mode then it enables the RxIrq and it goes to uart_recvchars(), where it checks whether data is available in rx fifo or not.
  • so to check the data in rx fifo, we need to send the data first, which we have sent through rtl8721d_up_send() and before sending the data we need to enable the g_tc_rxavailable so that g_tc_rxavailable_res can collect the result for our sent data and when it collects the result then immediately turn off the g_tc_rxavailable, which means it won't collect the result further.

please let me know what you think about this approach.
I'll add the comments once we agree on the approach.

Copy link
Contributor Author

@deepaksrma deepaksrma Sep 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kishore-sn @sangam-swami @jeongchanKim please review this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I understand they are "begin" and "end" of tc to get valid data. Am I right?
  2. It's the loopback test. There is no way to get another data. Why do you need begin and end?
  3. Even it's loopback mode, sender and receiver should be implemented using existed APIs without more variables. If it's impossible, it means our APIs are not enough to use serial functionality. Then we should add them as public API, not tc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about changing this implementation as follows:
Remove all global variables.

case TIOCS_AVAIL:
rtl8721d_up_send(dev, 'a');
sleep(1);
ret = rtl8721d_up_rxavailable(dev);
break;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, I think we can change it further to make the code much cleaner.

In the driver we do:
case TIOCS_AVAIL:
ret = rtl8721d_up_rxavailable(dev);
break;

In the test case file we do:
iotbus_uart_context_h h_uart = iotbus_uart_init(DEVPATH);
ret = iotbus_uart_control_loopback(h_uart, LOOPBACK_ON);
ret = iotbus_uart_write(h_uart, sz_input_text, sizeof(sz_input_text));
ret = iotbus_uart_rxavailable(h_uart);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can not do like this:

case TIOCS_AVAIL:
rtl8721d_up_send(dev, 'a');
sleep(1);
ret = rtl8721d_up_rxavailable(dev);
break;

because when we sent the data in loopback mode then it comes on the RX FIFO immediately and generates the interrupt,
and calls uart_recvchars() where it checks whether data is available in RX FIFO or not, if data is available then it removes the data from RX FIFO and copies it to buffer so the following lines won't work, because when it comes to rtl8721d_up_rxavailable(), then data is not in RX FIFO because it has been transferred to buffer.

ret = rtl8721d_up_rxavailable(dev);
break;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.
Then we need to keep a global variable. But its better to keep a single global variable and protect it with a semaphore. Add a new ioctl to reset global variable.

bool g_rxavail' 'sem_t g_rxavail_sem

TCIOC_RXAVAIL_RESET:
take g_rxavail_sem
g_rxavail = false
give g_rxavail_sem

TCIOC_RXAVAIL:
take g_rxavail_sem
ret = g_rxavail;
give g_rxavail_sem

And in rtl8721d_up_rxavailable()

take g_rxavail_sem
g_rxavail = ret;
give g_rxavail_sem

break;

case TIOCS_READY:
ret = rtl8721d_up_txready(dev);
break;

case TIOCS_EMPTY:
ret = rtl8721d_up_txempty(dev);
break;
Comment on lines +576 to +590
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. How about other boards like imxrt? Does it return invalid or not supported or already impleneted?
  2. Are they only used in the tc for coverage? Then let's cover them with TC config.

Copy link
Contributor Author

@deepaksrma deepaksrma Sep 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will add this in other boards after finalizing for this board


default:
ret = -ENOTTY;
break;
Expand Down Expand Up @@ -628,8 +646,17 @@ static void rtl8721d_up_rxint(struct uart_dev_s *dev, bool enable)

static bool rtl8721d_up_rxavailable(struct uart_dev_s *dev)
{
bool ret = 0;
struct rtl8721d_up_dev_s *priv = (struct rtl8721d_up_dev_s *)dev->priv;
DEBUGASSERT(priv);
if (g_tc_rxavailable) {
ret = (serial_readable(sdrv[uart_index_get(priv->tx)]));
if (!g_tc_rxavailable_res) {
g_tc_rxavailable_res = ret;
g_tc_rxavailable = 0;
}
return ret;
}
return (serial_readable(sdrv[uart_index_get(priv->tx)]));
}

Expand Down