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

[WIP] Add MQTT logging #641

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jbaudoux
Copy link
Contributor

@jbaudoux jbaudoux commented Mar 31, 2024

Forward messages to MQTT broker

Relates to #442

In your BSB_LAN_config.h, set

  • byte LoggingMode = 4; // CF_LOGMODE_MQTT
  • int logTelegram = LOGTELEGRAM_ON

@jbaudoux
Copy link
Contributor Author

What's the purpose of dest_addr in log_parameters ?
In MQTT, this adds a ! and the address in the topic but that does not correspond to any MQTT usage

@fredlcore
Copy link
Owner

Is there really no way that you can work on your contributions in your fork and only submit a PR when it's ready for review? Then these "draft PRs" wouldn't clutter my project's PR list which is basically my main ToDo list for this project.

And again: I don't entertain any questions before there is an explanation on what is planned to do and how it is planned to do it. Other than that, the questions you ask show that you haven't even made it to chapter 5 in the manual.

@jbaudoux jbaudoux force-pushed the imp-20240324-mqtt_log branch 2 times, most recently from def368a to e2a7b16 Compare March 31, 2024 14:09
@jbaudoux
Copy link
Contributor Author

jbaudoux commented Mar 31, 2024

First trial shows promising results. I tested with logTelegram = LOGTELEGRAM_ON
Now the MQTT broker is aware of any static config change. Whatever the thermostat I use, the SET command is forwarded with the new value. The state updates that are published on the bus are also forwarded to the MQTT broker (e;g. 10025, 10027, 10028).
I only need to list in log_parameters the moving temperature values that are not automatically notified on the bus like the outside temperature (8700) and room temperature (8740)

@fredlcore
Copy link
Owner

fredlcore commented Mar 31, 2024

No one - including me - knows what parameters you are referring to when you talk abount parameter numbers >= 10020...

Forward messages to MQTT broker
@fredlcore
Copy link
Owner

fredlcore commented Apr 10, 2024

Even though it's still in draft, please take note of the following:

  • Have a look at the function parsingStringToParameter.
  • We specifically introduced the parameter struct for easier handling of parameters as well as destinations. Do not introduce your own kind of handling of that by removing code that explicitly uses this struct, unless you have a good reason to do so.

Again, please explain what you plan to do before wasting your own time on a PR that I may not accept in the end.
I appreciate if someone takes the time to refactor this part of the code which has grown organically too entangled, but if we do such a major move, it should be done solidly:

  • query() should be removed from mqtt_sendtoBroker(). If the former is executed before the latter, we will have decodedTelegram populated with all relevant information, so it can be published right away.
  • query() should no longer be called with the line number but a parameter struct. Having also the destination device ID in the function would enable us to move a lot of the temporary switching of destination devices to the query() function, as well as (optionally) call mqtt_sendtoBroker() which requires a parameter struct to be passed to it.
  • printTelegram also uses decodedTelegram, so mqtt_sendtoBroker() could easily be called from there as well. There is also quite a significant amount of overlap between LogTelegram() and printTelegram(), and it might be worthwhile to bring together all log functionalities (debug output, SD card, MQTT, UDP) together in one function (or one with several sub functions).

if (LoggingMode & CF_LOGMODE_MQTT) {
printlnToDebug(PSTR("Send to MQTT"));
if ((decodedTelegram.prognr != -1) and (!is_query)) {
mqtt_sendtoBroker(-1);
Copy link
Owner

Choose a reason for hiding this comment

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

How do you know that the destination address is always the default destination address (that's what -1 stands for in param.dest_addr)?

if (bus->getBusType() != BUS_PPS) {
if (msg[4+(bus->getBusType()*4)]==TYPE_QUR) is_query = true;
Copy link
Owner

Choose a reason for hiding this comment

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

What about TYPE_QINF, TYPE_ACK, TYPE_NACK etc., do you want to log them as well? Do you know the state of decodedTelegram in these cases?

@fredlcore
Copy link
Owner

This has become obsolete now, I think. Thanks for the ideas anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants