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.

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

[HELP] psmq/embedlog build #2995

Closed
1 task done
LuchianMihai opened this issue Feb 14, 2025 · 8 comments
Closed
1 task done

[HELP] psmq/embedlog build #2995

LuchianMihai opened this issue Feb 14, 2025 · 8 comments
Labels
Community: Question Further information is requested

Comments

@LuchianMihai
Copy link
Contributor

Description

I'm trying to use psmq as an IPC method for my application.

There are some issues getting psmq and embedlog to work and I am trying to understand what would be an fix.

  • I get warnings about HAVE_TERMIOS_H:
/home/tk/nxspace/apps/include/system/embedlog.h:18:5: warning: "HAVE_TERMIOS_H" is not defined, evaluates to 0 [-Wundef]
   18 | #if HAVE_TERMIOS_H
      |     ^~~~~~~~~~~~~~

but that flag is set in the Makefile

CFLAGS += -DHAVE_TERMIOS_H

So there may be some issues with the build stage of embedlog?

  • Also, I guess some nuttx changes happened as well?
diff --git a/logging/embedlog/Makefile b/logging/embedlog/Makefile
index e4e6f2b4c..460f0d9b5 100644
--- a/logging/embedlog/Makefile
+++ b/logging/embedlog/Makefile
@@ -231,6 +231,7 @@ $(EMBEDLOG_SOURCES)/include/embedlog.h: $(EMBEDLOG_SOURCES)/include/embedlog.h.i
            -e "s/^#if n$$/#if 0/" $< > $@
 
 create_includes: $(EMBEDLOG_SOURCES)/include/embedlog.h
+       $(Q) $(CP) $< $(APPDIR)/include/system  
        $(Q) $(CP) $< $(APPDIR)/include/logging
 
 context:: $(EMBEDLOG_SOURCES)
diff --git a/system/psmq/Makefile b/system/psmq/Makefile
index b414da61e..9af9c746e 100644
--- a/system/psmq/Makefile
+++ b/system/psmq/Makefile
@@ -105,6 +105,7 @@ $(PSMQ_SOURCES)/inc/psmq.h: $(PSMQ_SOURCES)/inc/psmq.h.in
 
 create_includes: $(PSMQ_SOURCES)/inc/psmq.h
        $(Q) $(CP) $< $(APPDIR)/include/system
+       $(Q) $(CP) $< $(APPDIR)/include/logging
 
 context:: $(PSMQ_SOURCES)
        $(Q) $(MAKE) create_includes

create_includes rule needs to be extended as in diff above, but I do not know which change is correct

Can someone help me out? @mlyszczek

Verification

  • I have verified before submitting the report.
@LuchianMihai LuchianMihai added the Community: Question Further information is requested label Feb 14, 2025
@mlyszczek
Copy link
Contributor

I will look into this. Can you please share nuttx version and your .config?

@mlyszczek
Copy link
Contributor

Ok, I looked it up. Generally this warning is absolutely harmless. This is relic of the past when some functions were using speed_t for tty speed, so termios.h was required. It's been changed to unsigned ever since so that header is no needed. I will patch this out upstream so future versions don't throw that warning.

@LuchianMihai
Copy link
Contributor Author

LuchianMihai commented Feb 14, 2025

Hi, I've noticed a few things:

Regarding HAVE_TERMIOS_H
The issue comes from the fact that CFLAGS += -DHAVE_TERMIOS_H is provided only by the apps/logging/embedlog Makefile, which will offer the flag for embedlog app (or rather translation units). But embedlog.h is also used in psmq app, there is no CFLAGS += -DHAVE_TERMIOS_H in the apps/system/psmq Makefile, so, when building broker.c which import embedlog.h the preprocessor will not see the HAVE_TERMIOS_H, so it throws warnings.

Regarding includes
Both psmq and embedlog have this line

CFLAGS += ${INCDIR_PREFIX}$(APPDIR)$(DELIM)include$(DELIM)system

Both includes system, but the embedlog copies it's header to $(APPDIR)/include/logging, without including logging.
So my take is that the cflag for embedlog and/or psmq needs to be changed accordingly.

Another thing:
embedlog offers support for syslog, which is only used for nuttx based on your release notes (please correct me if i'm wrong).
But, I do not see syslog.h included which is needed for syslog function, maybe it gets added if autotools are used, but not during normal nuttx build. So syslog may not work at all. If my assumption is correct I can open an PR adding the required header file in el-private.h on your repo.

@LuchianMihai
Copy link
Contributor Author

Hopefully some of my statements have sense, if not ask and I will try to give more details.

@mlyszczek
Copy link
Contributor

mlyszczek commented Feb 14, 2025

Regarding HAVE_TERMIOS_H
The issue comes from the fact that CFLAGS += -DHAVE_TERMIOS_H is provided only by the apps/logging/embedlog Makefile, which will offer the flag for embedlog app (or rather translation units). But embedlog.h is also used in psmq app, there is no CFLAGS += -DHAVE_TERMIOS_H in the apps/system/psmq Makefile, so, when building broker.c which import embedlog.h the preprocessor will not see the HAVE_TERMIOS_H, so it throws warnings.

That is correct. And it doesn't matter since embedlog no longer uses anything from termios in API functions.

Regarding includes Both psmq and embedlog have this line

CFLAGS += ${INCDIR_PREFIX}$(APPDIR)$(DELIM)include$(DELIM)system

Both includes system, but the embedlog copies it's header to $(APPDIR)/include/logging, without including logging. So my take is that the cflag for embedlog and/or psmq needs to be changed accordingly.

That is problem with build system. And more precisely that embedlog was moved from system to logging, but paths were not updated. nuttx seems to be installing all headers into separate include directory which is different than on other posix OSes. On any other unix embedlog.h would live in /usr/include but in nuttx it lies in non standard path, hence the problem.

Another thing:
embedlog offers support for syslog, which is only used for nuttx based on your release notes (please correct me if i'm wrong).

That has already been patched in upstream some time ago I just did not push it. I will make a release with those fixes and update it in nuttx.

@mlyszczek
Copy link
Contributor

You can check those 2 patches

#2997
#2996

Fixed all warnings you mentioned + some bonus fixes. It builds without warning on my machine for stm32-nucleo target. If you have some more errors/warnings let me know.

@LuchianMihai
Copy link
Contributor Author

Thanks, will close this issue when the PRs gets merged.

@LuchianMihai
Copy link
Contributor Author

Both pull requests got merged. Closing ticket

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community: Question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants