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

Add motor enable 1.0x #140

Merged
merged 19 commits into from
Jul 6, 2023
Merged

Add motor enable 1.0x #140

merged 19 commits into from
Jul 6, 2023

Conversation

Kotochleb
Copy link
Contributor

@Kotochleb Kotochleb commented Jul 5, 2023

bump::patch

Port motors enable service behavior to Panther 1.06 and older

@Kotochleb Kotochleb requested a review from rafal-gorecki July 5, 2023 13:16
Comment on lines +182 to +184
motor_state = self._lines['STAGE2_INPUT'].get_value()
# if switch changes from off to on overwrite service value
if not self._last_motor_state and motor_state:
Copy link
Contributor

Choose a reason for hiding this comment

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

self._lines['STAGE2_INPUT'].get_value() bardzo często się pojawia w kodzie proponuje stworzyć lambde is_stage2()

Suggested change
motor_state = self._lines['STAGE2_INPUT'].get_value()
# if switch changes from off to on overwrite service value
if not self._last_motor_state and motor_state:
# if switch changes from off to on overwrite service value
if not self._last_motor_state and self.is_stage2():

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 appears 4 times. If we had to add lambda, we would have to do this for both pins and also port it to power_borad_node.py to keep the same coding style

if not self._last_motor_state and motor_state:
self._motor_enabled = True

self._last_motor_state = motor_state
Copy link
Contributor

Choose a reason for hiding this comment

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

Taught with C++ practice, I think that it would not be a stupid approach to use static variables in such situations. The _last_motor_state variable is really only needed in this function, so you can encapsulate such things so as not to clutter the whole class.

Example

def my_function(self):
    if not hasattr(self.my_function, "counter"):
        self.my_function.counter = 0

    self.my_function.counter += 1
    print(f"Counter: {self.my_function.counter}")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see the point. This makes the code less readable. We would have to make an strong assumption the class is a singleton. Even though it would work I do not think assigning variables to a function rather than object is a good idea

panther_power_control/src/relays_node.py Outdated Show resolved Hide resolved
@Kotochleb Kotochleb requested a review from rafal-gorecki July 6, 2023 14:58
@rafal-gorecki rafal-gorecki merged commit c5e9031 into ros1 Jul 6, 2023
@rafal-gorecki rafal-gorecki deleted the ros1-add-motor-enable-1.0x branch July 6, 2023 16:24
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