Skip to content

Conversation

@sputn1ck
Copy link
Member

@sputn1ck sputn1ck commented Aug 25, 2023

This PR adds a module for a finite state machine. The goal of the module is to provide a simple, easy to use, and easy to understand finite state machine. The module is designed to be used in future loop subsystems. Additionally a state visualizer is provided to help with understanding the state machine.

TODO

  • Track test coverage
  • Add more test coverage

Next PR: #632

@hieblmi hieblmi self-requested a review August 25, 2023 09:10
Copy link
Collaborator

@hieblmi hieblmi left a comment

Choose a reason for hiding this comment

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

Good job, I really like the idea of having a common state machine template for future projects so reviewing new features becomes a lot easier in that respect. How did you arrive at choosing this particular one? Did you write it yourself or do we need to add a license disclaimer etc?

Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

Concept ACK, PR is also almost ready imo. 🍔

fsm/fsm.go Outdated
Previous StateType

// Current represents the current state.
Current StateType
Copy link
Member

Choose a reason for hiding this comment

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

Given that SendEvent (and then getNextState) changes the Current's and Previous' value we might want to instead expose these as functions that also lock the mutex.

Copy link
Member Author

@sputn1ck sputn1ck Sep 6, 2023

Choose a reason for hiding this comment

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

The mutex is locked for the whole duration of SendEvent. I opted to not export current and previous. It can now only be 'read' by a registeard observer

@sputn1ck sputn1ck requested a review from bhandras September 6, 2023 08:18
@sputn1ck sputn1ck marked this pull request as ready for review September 6, 2023 08:19
Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

Looks good, just a few nits/questions remain from my end!

fsm/fsm.go Outdated
s.mutex.Lock()
defer s.mutex.Unlock()

fsmConfig := s.States
Copy link
Member

Choose a reason for hiding this comment

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

nit: do we need this local var?

Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

Almost ready!

Copy link
Collaborator

@hieblmi hieblmi left a comment

Choose a reason for hiding this comment

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

LGTM, left some comments. Can't wait to use the FSM for new features.

Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM FSM FTW 🥇

@sputn1ck sputn1ck requested a review from bhandras September 7, 2023 11:44
@sputn1ck sputn1ck force-pushed the instantloopout_1 branch 4 times, most recently from 1230386 to ef23fe2 Compare September 7, 2023 13:15
Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

💯

Copy link
Collaborator

@hieblmi hieblmi left a comment

Choose a reason for hiding this comment

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

LGTM!

This commit adds a module for a finite state machine. The goal of the
module is to provide a simple, easy to use, and easy to understand
finite state machine. The module is designed to be used in future
loop subsystems. Additionally a state visualizer is provided to
help with understanding the state machine.
@sputn1ck sputn1ck merged commit 077d702 into lightninglabs:master Sep 7, 2023
@sputn1ck sputn1ck deleted the instantloopout_1 branch September 7, 2023 15:47
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.

4 participants