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

ROS 2 - Add GPIO driver package #211

Merged
merged 28 commits into from
Nov 30, 2023
Merged

ROS 2 - Add GPIO driver package #211

merged 28 commits into from
Nov 30, 2023

Conversation

pkowalsk1
Copy link
Contributor

No description provided.

@pkowalsk1 pkowalsk1 changed the title ROS2 - Add GPIO driver package ROS 2 - Add GPIO driver package Nov 20, 2023
Copy link
Contributor

@KmakD KmakD left a comment

Choose a reason for hiding this comment

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

Missing eof lines,
We should think which methods should throw errors or simply print them. How we are planning to handle these in client code?
missing README for package

panther_gpiod/CMakeLists.txt Outdated Show resolved Hide resolved
panther_gpiod/include/panther_gpiod/gpio_driver.hpp Outdated Show resolved Hide resolved
panther_gpiod/include/panther_gpiod/gpio_driver.hpp Outdated Show resolved Hide resolved
panther_gpiod/include/panther_gpiod/gpio_driver.hpp Outdated Show resolved Hide resolved
panther_gpiod/include/panther_gpiod/gpio_driver.hpp Outdated Show resolved Hide resolved
panther_gpiod/include/panther_gpiod/gpio_driver.hpp Outdated Show resolved Hide resolved
panther_gpiod/src/gpio_driver.cpp Outdated Show resolved Hide resolved
panther_gpiod/src/gpio_driver.cpp Outdated Show resolved Hide resolved
panther_gpiod/src/gpio_driver.cpp Outdated Show resolved Hide resolved
panther_gpiod/include/panther_gpiod/gpio_driver.hpp Outdated Show resolved Hide resolved
pkowalsk1 and others added 5 commits November 21, 2023 15:38
Co-authored-by: Dawid Kmak <73443304+KmakD@users.noreply.github.com>
Co-authored-by: Dawid Kmak <73443304+KmakD@users.noreply.github.com>
@pkowalsk1 pkowalsk1 requested a review from KmakD November 23, 2023 09:32
Copy link
Contributor

@KmakD KmakD left a comment

Choose a reason for hiding this comment

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

still missing eol line. Have you run pre-commit?

panther_gpiod/CMakeLists.txt Outdated Show resolved Hide resolved
panther_gpiod/src/gpio_driver.cpp Show resolved Hide resolved
panther_gpiod/include/panther_gpiod/gpio_driver.hpp Outdated Show resolved Hide resolved
panther_gpiod/include/panther_gpiod/gpio_driver.hpp Outdated Show resolved Hide resolved
panther_gpiod/include/panther_gpiod/gpio_driver.hpp Outdated Show resolved Hide resolved
panther_gpiod/include/panther_gpiod/gpio_driver.hpp Outdated Show resolved Hide resolved
auto gpio_chip = gpiod::chip(gpio_chip_path_);
line_request_ = create_line_request(gpio_chip);

gpio_monitor_on();
Copy link
Contributor

Choose a reason for hiding this comment

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

Thing to discuss. Do we always assume that we want gpio monitor at the start and if this functionality is not needed then client code should turn it off. Or do we assume that this is disabled by default and client code is responsible for turning gpio monitor on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to me that this might be inconvenient for the user. When monitoring is turned off, the 'is_pin_active' method will not provide accurate information. Do we want to add a check in it to see if the thread is enabled? If so, what should it return then?

Copy link
Contributor

Choose a reason for hiding this comment

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

If is_pin_active cant work correctly without monitor it should check if it is running. Optionally it could turn it on if it is not enabled. Don't know if it is a good idea but you wouldn't need to turn it on in constructor.

Copy link
Contributor

@macstepien macstepien left a comment

Choose a reason for hiding this comment

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

I think that you didn't use precommit here.

panther_gpiod/include/panther_gpiod/gpio_driver.hpp Outdated Show resolved Hide resolved
panther_gpiod/include/panther_gpiod/gpio_driver.hpp Outdated Show resolved Hide resolved
panther_gpiod/include/panther_gpiod/gpio_driver.hpp Outdated Show resolved Hide resolved
panther_gpiod/include/panther_gpiod/gpio_driver.hpp Outdated Show resolved Hide resolved
panther_gpiod/include/panther_gpiod/gpio_driver.hpp Outdated Show resolved Hide resolved
panther_gpiod/src/gpio_driver.cpp Outdated Show resolved Hide resolved
panther_gpiod/src/gpio_driver.cpp Outdated Show resolved Hide resolved
@pkowalsk1 pkowalsk1 requested a review from macstepien November 28, 2023 13:33
@pkowalsk1 pkowalsk1 requested a review from macstepien November 28, 2023 13:39
panther_gpiod/src/gpio_driver.cpp Outdated Show resolved Hide resolved
@pkowalsk1 pkowalsk1 requested a review from macstepien November 28, 2023 14:25
Comment on lines 89 to 110
/**
* @brief Constructs the GPIODriver object with information about GPIO pin configurations.
* This information is necessary for initializing the GPIO functionality.
*
* @param gpio_info_storage Vector containing information about GPIO pin configurations.
*
* @throws std::runtime_error if the provided `gpio_info_storage` vector is empty.
*
* @par Example
* An example of constructing the GPIODriver object by providing GPIO pin information:
* @code{.cpp}
* std::vector<GPIOInfo> gpio_configurations = {
* GPIOInfo{GPIOPin::CHRG_SENSE, gpiod::line::direction::INPUT},
* GPIOInfo{GPIOPin::AUX_PW_EN, gpiod::line::direction::OUTPUT},
* GPIOInfo{GPIOPin::LED_SBC_SEL, gpiod::line::direction::OUTPUT, true,
* gpiod::line::value::ACTIVE}
* // ... additional GPIO pin configurations
* };
* GPIODriver gpio_driver(gpio_configurations);
* @endcode
*/
GPIODriver(std::vector<GPIOInfo> gpio_info_storage);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also add info that to enable reading pin values it is required to enable gpio monitoring, see GPIOMonitorEnable method for more info.

@pkowalsk1 pkowalsk1 requested a review from KmakD November 29, 2023 13:04
panther_gpiod/src/gpio_driver.cpp Outdated Show resolved Hide resolved
panther_gpiod/src/gpio_driver.cpp Outdated Show resolved Hide resolved
panther_gpiod/README.md Outdated Show resolved Hide resolved
pkowalsk1 and others added 3 commits November 29, 2023 15:17
Co-authored-by: Maciej Stępień <maciek1284@outlook.com>
Co-authored-by: Maciej Stępień <maciek1284@outlook.com>
@pkowalsk1 pkowalsk1 requested a review from macstepien November 30, 2023 09:15
@pkowalsk1 pkowalsk1 marked this pull request as ready for review November 30, 2023 10:01
@pkowalsk1 pkowalsk1 merged commit c06700c into ros2-devel Nov 30, 2023
@pkowalsk1 pkowalsk1 deleted the ros2-gpiod-driver-pkg branch November 30, 2023 10:02
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