Skip to content

SNS - High Power Relay - #81

Open
emmabaird wants to merge 14 commits into
masterfrom
HPRelaySensors
Open

SNS - High Power Relay#81
emmabaird wants to merge 14 commits into
masterfrom
HPRelaySensors

Conversation

@emmabaird

@emmabaird emmabaird commented Feb 26, 2024

Copy link
Copy Markdown

Referred to hardware.gpio and keyence which alsouses GPIO for 1st attempt to program to write to GPIO. There are a couple of cannot open source file and #include errors I'm not sure how to solve.

uses GPIO for 1st attempt to program to write to
GPIO.There are a couple of cannot open source
file and #include errors I'm not sure how to
solve.
@licornes-fluos licornes-fluos changed the title HPRelay SNS - HPRelay Feb 26, 2024
@TomLonergan03 TomLonergan03 changed the title SNS - HPRelay SNS - HP Relay Feb 27, 2024

@TomLonergan03 TomLonergan03 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You don't need to reimplement all the GPIO stuff. Take a IGpio in a static create function, then call getWriter on that and pass the writer to the constructor. See #72 for an example of what I mean. Then, you will just make Relay::setState() and Relay::getState() methods on the object, which call gpiowriter_.write().

You also might as well rename it all to be for a generic relay, so Relay instead of HPRelay cos we'll have a few of them around

@TomLonergan03 TomLonergan03 changed the title SNS - HP Relay SNS - High Power Relay Feb 27, 2024
feedback.
Aware will not pass all tests as having
clang-format problems on laptop -looking
into this.

@TomLonergan03 TomLonergan03 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

good start! I think there's a nicer abstraction where the client interacts with a relay by simply doing

auto optional_relay = Sensors::Relay::create(/* args */);
// do the optional checks
auto relay = *optional_relay;
relay.close();
// do the high power stuff or whatever
relay.open();

Comment thread lib/sensors/HPRelay.cpp Outdated
Comment thread lib/sensors/HPRelay.hpp Outdated
Comment thread lib/sensors/Relay.cpp Outdated
Relay::~Relay() {
}

std::optional<RelayWriter> RelayWriter::getWriter(const std::uint8_t pin,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this function isn't needed

Comment thread lib/sensors/Relay.cpp Outdated
RelayWriter::~RelayWriter() {
}

bool RelayWriter::Write() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

return type should be core::Result, also refactor to be like:

core::Result Relay::open() {
    // set relay to be open by writing the GPIO signal if needed
}

and similarly a Relay::close() method

Comment thread lib/sensors/Relay.hpp Outdated
Relay(core::ILogger& logger, std::shared_ptr<io::IGpioWriter> gpio_writer);
};

class RelayWriter {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you can get rid of this whole class, it isn't needed

Comment thread lib/sensors/Relay.cpp Outdated
}

bool RelayWriter::Write() {
core::DigitalSignal result = gpio_writer_->write();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This returns a core::Result type I believe, so you can simply do return result; instead of needing the conditionals

@licornes-fluos

Copy link
Copy Markdown
Contributor

Change file names to relay.hpp and relay.cpp

@licornes-fluos licornes-fluos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you should start from scratch and look at #72.
Your code is overcomplicated and somehow doesn't use the protocols properly.
You need to call methods from the gpio protocol (hyped-2024/lib/io/gpio.hpp) and use those in your code.

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.

3 participants