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

Python Mqtt and exception cleanup #1701

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Python Mqtt and exception cleanup #1701

wants to merge 18 commits into from

Conversation

jmthomas
Copy link
Member

@jmthomas jmthomas commented Nov 12, 2024

closes #1197

Sorry for the noise in there about the exceptions but I started in on it during MQTT implementation and it all sort of rolled together. I removed the ssl parameter because I don't think it makes sense by itself and only applies if an option is set. That is breaking but this is COSMOS 6 and I think the MQTT usage is pretty low.

Copy link

codecov bot commented Nov 12, 2024

Codecov Report

Attention: Patch coverage is 95.12195% with 4 lines in your changes missing coverage. Please review.

Project coverage is 76.60%. Comparing base (792980a) to head (2f4fb99).
Report is 33 commits behind head on main.

Files with missing lines Patch % Lines
openc3/lib/openc3/streams/serial_stream.rb 80.00% 2 Missing ⚠️
openc3/lib/openc3/models/interface_model.rb 0.00% 1 Missing ⚠️
openc3/lib/openc3/models/tool_model.rb 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1701      +/-   ##
==========================================
+ Coverage   76.31%   76.60%   +0.28%     
==========================================
  Files         603      608       +5     
  Lines       46101    46494     +393     
  Branches      844      844              
==========================================
+ Hits        35183    35617     +434     
+ Misses      10821    10780      -41     
  Partials       97       97              
Flag Coverage Δ
python 84.21% <ø> (+0.24%) ⬆️
ruby-api 48.61% <ø> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jmthomas jmthomas changed the title Mqtt Python Mqtt and exception cleanup Nov 13, 2024
Copy link
Member

@ryanmelt ryanmelt left a comment

Choose a reason for hiding this comment

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

Please restore ssl parameter. If only ssl parameter is given, you can pull Ca file if necessary from ENV['SSL_CERT_FILE']

@@ -91,12 +92,11 @@ module OpenC3
class MqttInterface < Interface
# @param hostname [String] MQTT server to connect to
# @param port [Integer] MQTT port
# @param ssl [Boolean] Use SSL true/false
def initialize(hostname, port = 1883, ssl = false)
Copy link
Member

Choose a reason for hiding this comment

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

Please restore ssl parameter

require 'openc3/interfaces/stream_interface'
require 'openc3/streams/mqtt_stream'

module OpenC3
class MqttStreamInterface < StreamInterface
# @param hostname [String] MQTT server to connect to
# @param port [Integer] MQTT port
# @param ssl [Boolean] Use SSL true/false
def initialize(hostname, port = 1883, ssl = false, write_topic = nil, read_topic = nil, protocol_type = nil, *protocol_args)
Copy link
Member

Choose a reason for hiding this comment

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

restore ssl parameter

@@ -33,27 +33,61 @@ class MqttStream < Stream
attr_accessor :key
attr_accessor :ca_file

def initialize(hostname, port = 1883, ssl = false, write_topic = nil, read_topic = nil)
Copy link
Member

Choose a reason for hiding this comment

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

restore ssl

@jmthomas
Copy link
Member Author

jmthomas commented Nov 29, 2024

Please restore ssl parameter. If only ssl parameter is given, you can pull Ca file if necessary from ENV['SSL_CERT_FILE']

There is no code which does this. Is this another else case if cert, key and ca_file are all not set?

@ryanmelt
Copy link
Member

I should be able to use mqtt with a https:// url and not provide anything else.

Copy link

sonarcloud bot commented Nov 29, 2024

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.

Python MQTT interface
2 participants