KDMqtt #45

marcothaller · 2024-11-26T09:46:28Z

Draft PR to gather feedback / review of KDMqtt.

Re-enable as soon as KDMqtt is supported and tested on macos.

KDMqtt provides a cross-platform MQTT client solution based on mosquitto. KDMqtt seamlessly integrates into KDFoundation's `CoreApplication` and comes with an API featuring properties and signals using KDBindings.

Adds a very basic MQTT client example. Examlple shows how to use and integrate KDMqtt in you KDFoundation CoreApplication. Example connects to broker at test.mosquitto.org, subscribes to mytopic as soon as connected to the broker and publishes a message for mytopic as soon as subscribing to the topic is done. The received message is then printed using spdlog::info. This example also outlines the steps neccessary when deploying an application using KDMqtt on windos (copying DLLs next to the binary).

In KDMqtt we use `mosquitto_ssl_get()` which was added to mosquitto 2020-09-10 and is first available in v2.0.20 Ubuntu 20.04 ships with mosquitto v1.6.9 Therefore, without changes to KDMqtt, KDMqtt cannot be built on Ubuntu 20.04 atm. Disable CI build (for now).

* add missing header include * add macOS specific paths to FindMosquitto.cmake * (re)enable CI run on macOS

for more information, see https://pre-commit.ci

MiKom · 2024-11-26T10:09:05Z

I will add more comments later, but first quick one is about logging.
I think it's better to use SPDLOG_LOGGER_* macros for logging because they already support printing the source location. I'd also create a dedicated logger for the mqtt system (see core_application.cpp)

So, you then can replace this:

spdlog::error("MqttClient::EventLoopHook::engage() - EventLoopHook is not initialized. Call MqttClient::EventLoopHook::init() first.");

with this:

SPDLOG_LOGGER_ERROR(m_logger, "EventLoopHook is not initialized. Call MqttClient::EventLoopHook::init() first.");

MiKom · 2024-11-26T14:04:14Z

It seems that you also need to add the dependency installation in the .github/workflows/linters.yml file

MiKom · 2024-11-30T15:14:57Z

examples/mqtt_client/CMakeLists.txt

+
+if(WIN32)
+    # Deployment: On Windows, copy all DLLs from the mosquitto install directory next to the application binary so that they're found.
+    if(BUILD_INTEGRATION_MQTT AND MOSQUITTO_RUNTIME_DLLS)


BUILD_INTEGRATION_MQTT doesn't seem to exist anymore so this condition is always false on Windows.

MiKom · 2024-11-30T15:18:33Z

CMakeLists.txt

@@ -60,6 +60,7 @@ endif()
 add_subdirectory(src/KDUtils)
 add_subdirectory(src/KDFoundation)
 add_subdirectory(src/KDGui)
+add_subdirectory(src/KDMqtt)


I'd make building KDMqtt optional behind an option. And this option should be by default OFF on Windows as currently, the example doesn't seem to work. Probably something's wrong with the fd notifiers? Dunno. We can create an issue to fix it.

Our current target is Linux anyway so for now it should be fine to have KDMqtt working on Linux only.

Some KDUTILS_BUILD_MQTT_SUPPORT

MiKom · 2024-11-30T15:19:21Z

.github/workflows/build.yml

@@ -43,7 +46,20 @@ jobs:
          sudo apt update -qq
          sudo apt install -y libxkbcommon-dev libxcb-xkb-dev \
          libxkbcommon-x11-dev wayland-scanner++ wayland-protocols \
-          libwayland-dev xvfb ninja-build
+          libwayland-dev xvfb ninja-build \
+          libmosquittopp-dev


IIUC we're not using the C++ mosquitto wrapper but rather the C API directly. In this case, this should be libmosquitto-dev

MiKom · 2024-11-30T15:20:05Z

.github/workflows/build.yml

@@ -15,7 +15,7 @@ jobs:
        os:
          - ubuntu-24.04
          - ubuntu-22.04
-          - ubuntu-20.04
+          #- ubuntu-20.04 -> disabled during introduction of KDMqtt


Is this still the case, that it doesn't work on 20.04?

MiKom · 2024-11-30T15:21:04Z

README.md

+You can find setup instructions and download links at
+
+<https://mosquitto.org/download/>
+


I'd also encourage the user to look at .github/workflows/build.yml

MiKom · 2024-11-30T19:25:38Z

src/KDMqtt/CMakeLists.txt

+
+target_include_directories(
+    KDMqtt
+    PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../..>


this makes mosquitto_wrapper.h a public header. Shouldn't we rather make it an implementation detail and private?

MiKom · 2024-11-30T19:32:17Z

src/KDMqtt/mqtt.h

+    friend class MqttUnitTestHarness;
+
+public:
+    MqttClient(const std::string &clientId, bool cleanSession = true, bool verbose = false);


I'd rather turn the two bool parameters into a single Flags parameter for better readability on call site.

MiKom · 2024-11-30T19:33:30Z

src/KDMqtt/mqtt.cpp

+    : m_verbose{ verbose }
+{
+    if (!MqttLib::instance().isInitialized()) {
+        spdlog::warn("MqttClient::MqttClient() - CTOR called before MqttLib::init(). Initialize lib before instantiating MqttClient object!");


That's an error IMO. The class can't really function properly without MqttLib being initialized.

MiKom · 2024-11-30T19:35:24Z

src/KDMqtt/mqtt.h

+
+private:
+    MqttLib();
+    ~MqttLib() = default;


I think that this class should call cleanup() on destruction. So that the user doesn't have to think about it (case in point: cleanup is not done in the example :) ).

According to mosquitto docs, mosquitto_lib_cleanup cannot fail so we can disregard its return value.

MiKom · 2024-11-30T19:40:45Z

src/KDMqtt/mqtt.h

+
+    KDBindings::Signal<> error;
+
+    virtual int setTls(const File &cafile) = 0;


We should also add the function that lets the user use the system certificate store for TLS.

I think that by default, we should use the system store by calling mosquitto_int_option with the MOSQ_OPT_TLS_USE_OS_CERTS option and let the user opt out of it somehow.

But also, remember to document that it only works on Linux because on Windows, OpenSSL used by mosquitto doesn't use the system store by default.

MiKom · 2024-11-30T19:45:04Z

src/KDMqtt/mqtt.cpp

+
+    if (!m_isInitialized) {
+        result = m_mosquittoLib->init();
+        const auto hasError = checkMosquittoResultAndDoDebugPrints(result, "MqttLib::init()");


This function is quite verbose and requires us to re-state the function name. I think we could replace it with a macro that would automatically handle source location and honour SPDLOG compile-time log enabling/disabling:

#if SPDLOG_ACTIVE_LEVEL <= SPDLOG_LEVEL_ERROR #define LOG_MOSQUITTO_ERROR(logger, file, line, func, error_string) \ (logger)->log(spdlog::source_loc{ file, line, func }, spdlog::level::err, error_string) #else #define LOG_MOSQUITTO_ERROR (void)0 #endif static bool checkAndLogMosquittoResult(std::shared_ptr<spdlog::logger> logger, int ec, const char *file, int line, const char *func) { const bool isError = (ec != MOSQ_ERR_SUCCESS); if (isError) { LOG_MOSQUITTO_ERROR(logger, file, line, func, mosquitto_strerror(ec)); } return isError; } #define CHECK_AND_LOG_MOSQUITTO_RESULT(ec) checkAndLogMosquittoResult(m_logger, ec, __FILE__, __LINE__, __FUNCTION__)

Then this callsite becomes:

const auto hasError = CHECK_AND_LOG_MOSQUITTO_RESULT(result);

Once we go to c++20 the macros of course will be gone as we'll be able to replace this with std::source_location

marcothaller requested a review from MiKom November 26, 2024 09:46

marcothaller added 9 commits November 26, 2024 10:54

Add mosquitto to linux and windows CI runners

5fe2804

Disable macos CI runner for now

2fceef9

Re-enable as soon as KDMqtt is supported and tested on macos.

Add FindMosquitto.cmake file

e9d1fad

Add initial iteration of KDMqtt

1aded3f

KDMqtt provides a cross-platform MQTT client solution based on mosquitto. KDMqtt seamlessly integrates into KDFoundation's `CoreApplication` and comes with an API featuring properties and signals using KDBindings.

Add KDMqtt specific infos to README

b58bc2c

Add export of KDMqtt symbols

6b3ad02

Support building KDMqtt on macOS

88b41d3

* add missing header include * add macOS specific paths to FindMosquitto.cmake * (re)enable CI run on macOS

marcothaller force-pushed the kdmqtt branch from 0940cf5 to 88b41d3 Compare November 26, 2024 09:59

[pre-commit.ci] auto fixes from pre-commit.com hooks

5df9507

for more information, see https://pre-commit.ci

MiKom requested changes Nov 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KDMqtt #45

KDMqtt #45

marcothaller commented Nov 26, 2024

MiKom commented Nov 26, 2024

MiKom commented Nov 26, 2024

MiKom Nov 30, 2024

MiKom Nov 30, 2024

MiKom Nov 30, 2024

MiKom Nov 30, 2024

MiKom Nov 30, 2024

MiKom Nov 30, 2024

MiKom Nov 30, 2024

MiKom Nov 30, 2024

MiKom Nov 30, 2024

MiKom Nov 30, 2024

MiKom Nov 30, 2024 •

edited

Loading

MiKom Nov 30, 2024

		You can find setup instructions and download links at

		<https://mosquitto.org/download/>


		KDBindings::Signal<> error;

		virtual int setTls(const File &cafile) = 0;

KDMqtt #45

Are you sure you want to change the base?

KDMqtt #45

Conversation

marcothaller commented Nov 26, 2024

MiKom commented Nov 26, 2024

MiKom commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiKom Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiKom Nov 30, 2024 •

edited

Loading