Skip to content

feat: create mqtt client#26

Merged
Alxandr merged 40 commits intomainfrom
feat/client
Dec 17, 2022
Merged

feat: create mqtt client#26
Alxandr merged 40 commits intomainfrom
feat/client

Conversation

@Alxandr
Copy link
Contributor

@Alxandr Alxandr commented Nov 27, 2022

BREAKING-CHANGE: rename most of the crates

Remaining work:

  • Into<Topic> for &CommandTopic and &StateTopic.
  • Allow explicit control over MQTT topics.
  • Fix broken doc links

@Alxandr
Copy link
Contributor Author

Alxandr commented Nov 28, 2022

@arcnmx do you care about (temporarily) dropping Windows support?

@arcnmx
Copy link
Contributor

arcnmx commented Nov 29, 2022

do you care about (temporarily) dropping Windows support?

Heh, personally my use-case already requires dbus and systemd, so I’m not exactly affected!

However, I do have a few things to note about paho-mqtt:

  1. Without setting default-features = false it won’t be possible to turn off the bundled feature, which is bad form for a library crate. I think the solution here is to expose a paho-bundled = ["paho-mqtt?/bundled"] feature, and include it in mqtt-client's default feature.
  2. Rust-native alternatives like rumqttc may be worth considering, I’ve been thinking about switching to it recently.
  3. If you really want to go insane with generics, build the whole thing on top of futures::{Stream,Sink} and make the mqtt backend pluggable :P
    EDIT: oh the latest commit kinda already did this, neat!

@Alxandr
Copy link
Contributor Author

Alxandr commented Nov 29, 2022

I think keeping paho as default feature is (at least for now) a good idea - as it's the only way you can use the library (I've - for now - sealed the traits). I looked at rumqttc - but they claimed that v5 support wasn't there yet - so I figured I'd keep the door open by going a trait route, but not including it at the moment.

Basically - as it stands, hass-mqtt-client is cfg(unix) only - the only reason to make paho optional is so that CI doesn't fail on windows.

@Alxandr
Copy link
Contributor Author

Alxandr commented Nov 29, 2022

Welp - I'm back to windows builds failing (shocker)... Turns out that target.cfg(unix).features does not work :-/. Guess I'll have to figure out how to allow the windows builds to fail.

@arcnmx
Copy link
Contributor

arcnmx commented Nov 30, 2022

Turns out that target.cfg(unix).features does not work :-/.

This should work just fine:

[target.'cfg(not(target_env = "msvc"))'.dependencies]
paho-mqtt = { version = "0.11", default-features = false, optional = true }

[features]
default = ["paho", "paho-bundled"]
paho = ["dep:paho-mqtt"]
paho-bundled = ["paho-mqtt?/bundled"]

(cfgs in the source will need adjustment to match, since the presence of the paho feature doesn't mean the dependency is actually enabled anymore - either include target_env in all cfgs referencing paho or use a build script to set a common cfg var)


but they claimed that v5 support wasn't there yet - so I figured I'd keep the door open by going a trait route, but not including it at the moment

Given that (afaik) brokers like mosquitto are backward-compatible and can accept clients of either version, and even home-assistant defaults to v3.1.1, and that going the trait route means you probably want to generally support the lowest common denominator of features, and that the most meaningful v5 feature is user properties that help clients avoid keeping local state... I don't think you really lose much by not having v5?

I'm not against paho or anything, I'm just slightly interested in dropping the C dependency myself. Either way, I like the trait approach.


btw, entirely unrelated question, why do you not like mod.rs?

@Alxandr
Copy link
Contributor Author

Alxandr commented Dec 1, 2022

I also have no love for C dependencies 😛. As for MQTT v5 - I don't remember anymore why I thought that was required - so it may not be a big issue. The MQTT traits required for hass-mqtt-client will be pretty small - so we can see once that's "done" what features it actually uses.

As for mod.rs, there's a couple of reasons I don't "like" it (I have nothing against it mind you):

  1. Consistency - I would either go for always using mod.rs for modules, or never.
  2. "Magic" - mod.rs has a "special" meaning that people have to know about. Compared to not using it, you will always find a file with the matching name of the mod statement. lib.rs suffers from the same problem - but there's still one less at lest.
  3. The directory structure you see in this project is what rust-analyzer creates for me. I don't make the files myself. I write mod foo, and then press control+. on it, and select "create module" (or whatever the option is called).

As someone who also does a lot of frontend (js/ts) development - I know some of the issues that can arise from all your components living in index.js files - so I prefer keeping distinct file-names where I can.

@Alxandr
Copy link
Contributor Author

Alxandr commented Dec 2, 2022

@arcnmx not sure if you're interested in the hass-mqtt-client or not - but I think the public API is about where I want it for now (though I may have forgotten parts). The implementation however..... is probably to be replaced. It needs to be refactored and cleaned. But I think I want to try building on it a bit first.

Also - the entire topic router was just something I wrote mostly for fun, to see if I could figure something cool out. I'm probably going to replace it in it's entirely with a HashMap<String, Vec<T>> - but it was fun to figure out :).

kodiakhq[bot]
kodiakhq bot previously approved these changes Dec 13, 2022
kodiakhq[bot]
kodiakhq bot previously approved these changes Dec 13, 2022
kodiakhq[bot]
kodiakhq bot previously approved these changes Dec 13, 2022
kodiakhq[bot]
kodiakhq bot previously approved these changes Dec 15, 2022
kodiakhq[bot]
kodiakhq bot previously approved these changes Dec 15, 2022
kodiakhq[bot]
kodiakhq bot previously approved these changes Dec 15, 2022
@Alxandr
Copy link
Contributor Author

Alxandr commented Dec 15, 2022

@arcnmx this PR has gotten enough messages that github has started hiding the middle 😛 - that being said I don't remember anymore all of the things I was going to do, so I added a few checkboxes to the first message. The only point I currently have left is allowing you to control the MQTT topics if you so desire. Not landed entirely on how yet, but I'm about to start looking at that. Once that's done I'm thinking I'll merge this, and you can open issues for things that are missing, cause it's so much easier to keep track off than a long list of text.

kodiakhq[bot]
kodiakhq bot previously approved these changes Dec 15, 2022
@Alxandr Alxandr merged commit bad7343 into main Dec 17, 2022
@Alxandr Alxandr deleted the feat/client branch December 17, 2022 21:35
@github-actions github-actions bot mentioned this pull request Dec 17, 2022
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.

2 participants