Skip to content

Allow looping with all actions #2

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

niksilver
Copy link

I don't know if you're still maintaining this, but I have found this addition useful. It does not change any existing defined behaviour, and it adds (what I think is) useful behaviour.

This change allows the user to define an event from and to the same state, while also ensuring all the callbacks get called. It preserves the existing behaviour of an event which has a from state but no to state, which returns a NO_TRANSITION.

@muescha
Copy link

muescha commented Oct 19, 2023

Q1: when this is useful? you have an example?

Q2: this is the only place where the magic state number used as a value, but setting it to 5 is it problematic when there is a state with number 5?

I think it would better test for nil (all the way through the fsm)?

@niksilver
Copy link
Author

Thanks for taking a look at this and asking the questions.

Is this useful? Do I have an example?

I am using it in this code here. This is part of an application released here. It runs on hardware with six buttons. Three are on/off keys (k1, k2, k3) and three are rotating encoders (e1, e2, e3). The application allows the user to record music, stop, play and move the record/play head backwards and forwards. It is ideal for a finite state machine.

One scenario for this application is that the user may be recording, realise they've made a mistake, and want to restart the recording immediately. I want them to be able to do this by simply hitting "record" again, not by having to hit "stop" first. Then if they do go from the record state to the record state (which I want to allow) the application has to tidy up from their first recording (a call to the leave_record() function) and then initialise a new recording (a call to the enter_record() function). The existing fsm code does not trigger these two functions, but my amendment does.

Can we do better than using a magic state 5? Wouldn't it be better to just check for nil all the way through fsm?

Yes, I agree it would be better to check for nil all the way through, and I have spent a bit of time understanding how to do that. Unfortunately I do think that will work. (You may be able to tell me I am wrong; I do not have much experience with Lua.)

One specific problem here is states_for_event, which maps from an event and a from state to a to state. If we allow states_for_event[event][from] == nil then we cannot distinguish between (a) a specific event that has a from state but no to state, and (b) a specific event that is not permitted from the from state. This becomes a problem in self.can(), for example.

So instead of checking for nil throughout fsm I can think of two solutions.

The first solution is to use a magic to state name to indicate a no-op (scenario (a) above). I proposed the magic name 5, but I agree this is not ideal. However, we do have * as a magic state name, so there is a precedent. Also, state names are intended to be used in function names, and there is already a limitation on permissible characters there (alphanumerics and underscore). So perhaps we can use a value outside of that limitation. Perhaps a single space or a # or similar would be acceptable instead of 5.

The second solution is to separately track all the no-op states. This might logically clear but would add a lot of code - an entirely new table that always needs to be checked. This would make fsm less readable and less maintainable.

So I would propose replacing 5 with a single space (or similar) to indicate a no-op event.

@muescha
Copy link

muescha commented Oct 22, 2023

  1. I saw a workaround for your specific situation. This was resolved by adding an extra state called "restart recording," which then triggers the next event automatically.

s:record -> e:k2_long_press -> s:restart_record -> e:restart -> s:record

  1. Yes! A symbol that isn't permitted as a function name would be a more suitable choice :) Space isn't ideal, but other options like #@% would work better :)

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