daniel-p-carvalho commented on code in PR #3791:
URL: https://github.com/apache/nuttx-apps/pull/3791#discussion_r4057453090
##########
netutils/ptpd/ptpd.c:
##########
@@ -116,6 +122,13 @@ struct ptp_state_s
int tx_socket;
+ /* Hardware TX timestamp retrieval: consecutive failures, and whether it
+ * was given up on because the driver does not provide the timestamps.
+ */
+
+ unsigned int hwts_tx_failures;
Review Comment:
The detection is there because `CONFIG_NET_TIMESTAMP` only says that the
network stack can carry timestamps, not that the driver returns one for
transmitted frames. There is no way to know that at build time: the stack has a
flag for drivers that stamp received packets (`NETDEV_RX_STAMP`) but none for
transmitted ones, and only the sim driver stamps TX in software. So with
`CONFIG_NET_TIMESTAMP=y` and a driver without TX timestamps (for example the
STM32 driver without `CONFIG_STM32_ETH_TIMESTAMP_TX`) the error queue stays
empty and every event message waits for the whole poll timeout (500 ms) in the
main loop. That is what I saw on the board, and the reason for giving up after
a few failures. `ptpd` cannot look at the driver options instead, since they
belong to the architecture.
You are right that in a closed system the integrator knows which driver is
used. The capability itself also does not change while the system runs, so the
detection only decides once whether to use the hardware timestamps. The
alternatives I see:
1. Keep the detection as it is: three consecutive failures, then a warning
and software timestamps.
2. A Kconfig option of `ptpd` that says the driver returns TX timestamps,
without any detection. Less code, but a wrong setting brings the waits back and
nothing recovers from them.
3. Give up at the first failure. The least code. The price is that a failure
that is not permanent would switch hardware timestamps off until restart: the
driver can miss a timestamp while working (no IOB for the clone under load, or
a frame that ends without the timestamp bit set), and the software timestamp is
much less accurate (tens of microseconds on the board I tested).
I kept 1 because it is the safest, but I can change to 2 or 3 if you prefer.
##########
netutils/ptpd/ptpd.c:
##########
@@ -880,11 +899,103 @@ static int ptp_check_multicast_status(FAR struct
ptp_state_s *state)
return OK;
}
+#if defined(CONFIG_NET_TIMESTAMP) && defined(SO_TIMESTAMPING)
Review Comment:
Done, the condition is `#ifdef CONFIG_NET_TIMESTAMP` now (`SO_TIMESTAMPING`
is always defined). I changed the same condition in the other places of the
file too.
##########
netutils/ptpd/ptpd.c:
##########
@@ -880,11 +899,103 @@ static int ptp_check_multicast_status(FAR struct
ptp_state_s *state)
return OK;
}
+#if defined(CONFIG_NET_TIMESTAMP) && defined(SO_TIMESTAMPING)
+/****************************************************************************
+ * Name: ptp_get_tx_timestamp
+ *
+ * Description:
+ * Retrieve the hardware TX timestamp delivered via MSG_ERRQUEUE on the
+ * socket after transmission.
+ *
+ * Input Parameters:
+ * state - Pointer to PTP daemon state
+ * tx_ts - Location to return the hardware timestamp
+ *
+ * Returned Value:
+ * OK on success; ERROR on failure or timeout.
+ *
+ ****************************************************************************/
+
+static int ptp_get_tx_timestamp(FAR struct ptp_state_s *state,
+ FAR struct timespec *tx_ts)
+{
+ struct pollfd pfd;
+ int ret;
+
+ pfd.fd = state->tx_socket;
+ pfd.events = POLLPRI;
+ pfd.revents = 0;
+
+ ret = poll(&pfd, 1, 500);
+ if (ret > 0 && (pfd.revents & (POLLPRI | POLLERR)) != 0)
+ {
+ char errbuf[128];
+ char cmsgbuf[128];
+ struct msghdr msg;
+ struct iovec iov;
+ FAR struct cmsghdr *cmsg;
+ ssize_t n;
+
+ memset(&msg, 0, sizeof(msg));
+ iov.iov_base = errbuf;
+ iov.iov_len = sizeof(errbuf);
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+ msg.msg_control = cmsgbuf;
+ msg.msg_controllen = sizeof(cmsgbuf);
+
+ n = recvmsg(state->tx_socket, &msg, MSG_ERRQUEUE);
+ if (n >= 0)
+ {
+ for (cmsg = CMSG_FIRSTHDR(&msg); cmsg != NULL;
+ cmsg = CMSG_NXTHDR(&msg, cmsg))
+ {
+ if (cmsg->cmsg_level == SOL_SOCKET &&
+ cmsg->cmsg_type == SO_TIMESTAMPING)
+ {
+ FAR struct timespec *ts =
+ (FAR struct timespec *)CMSG_DATA(cmsg);
+
+ *tx_ts = ts[2];
+ return OK;
+ }
+ }
+
+ ptpwarn("PTP TX HWTS: recvmsg %zd B without SO_TIMESTAMPING\n",
+ n);
+ }
+ else
+ {
+ ptpwarn("PTP TX HWTS: recvmsg MSG_ERRQUEUE failed errno=%d\n",
+ errno);
+ }
+ }
+ else
+ {
+ ptpwarn("PTP TX HWTS: poll ret=%d revents=0x%04x errno=%d\n",
+ ret, pfd.revents, errno);
+ }
+
+ return ERROR;
+}
+#endif
+
static int ptp_sendmsg(FAR struct ptp_state_s *state, FAR const void *buf,
size_t buflen, FAR const void *addr,
socklen_t addrlen, FAR struct timespec *sendts)
{
int ret;
+ struct timespec sw_ts;
+#if defined(CONFIG_NET_TIMESTAMP) && defined(SO_TIMESTAMPING)
Review Comment:
Done, the condition is `#ifdef CONFIG_NET_TIMESTAMP` now (`SO_TIMESTAMPING`
is always defined). I changed the same condition in the other places of the
file too.
##########
netutils/ptpd/ptpd.c:
##########
@@ -938,9 +1049,48 @@ static int ptp_sendmsg(FAR struct ptp_state_s *state, FAR
const void *buf,
msg.msg_control = NULL;
msg.msg_controllen = 0;
+#if defined(CONFIG_NET_TIMESTAMP) && defined(SO_TIMESTAMPING)
Review Comment:
Done, the condition is `#ifdef CONFIG_NET_TIMESTAMP` now (`SO_TIMESTAMPING`
is always defined). I changed the same condition in the other places of the
file too.
##########
netutils/ptpd/ptpd.c:
##########
@@ -951,7 +1101,38 @@ static int ptp_sendmsg(FAR struct ptp_state_s *state, FAR
const void *buf,
if (sendts != NULL)
{
- ptp_gettime(state, sendts);
+#if defined(CONFIG_NET_TIMESTAMP) && defined(SO_TIMESTAMPING)
Review Comment:
Done, the condition is `#ifdef CONFIG_NET_TIMESTAMP` now (`SO_TIMESTAMPING`
is always defined). I changed the same condition in the other places of the
file too.
##########
netutils/ptpd/ptpd.c:
##########
@@ -2311,15 +2497,47 @@ int ptpd_start(FAR const struct ptpd_config_s *config)
if (pollfds[0].revents)
{
- /* Receive time-critical packet, potentially with cmsg
- * indicating the timestamp.
+#if defined(CONFIG_NET_TIMESTAMP) && defined(SO_TIMESTAMPING)
Review Comment:
Done, the condition is `#ifdef CONFIG_NET_TIMESTAMP` now (`SO_TIMESTAMPING`
is always defined). I changed the same condition in the other places of the
file too.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
