Receive [](start = 42, length = 7)

Past tense is better. As original 'Got'. Many time below. #Closed

fixed.

[](start = 32, length = 1)

wrong indentation. #Closed

fixed.

[](start = 33, length = 3)

inconsistent indentation #Closed

fixed.

platform_sfputil.get_transceiver_change_event [](start = 29, length = 45)

You can call platform_sfputil.get_transceiver_change_event function only once, and implement the logic as a normal state machine.

I don't mean your implementation is wrong, state machine may be much more readable and future proof. #Closed

Hi Qi,
I admit that rewriting it in a state machine approach can make the code a little shorter by eliminating some duplicate code.
But I'm afraid it can also introduce negative effects: We have the logic to control retry timespan and times and introduce some local variables to control that. Rewriting it as a state machine will combine the retry control logic into the main procedure of xcvrd, which make the main function complicated. #Closed

The benefit for state machine pattern is not shorter code, but a correct design and implementation.

For the design part, you need to consider any sequence of change events returned by vendor plugin code. For example, SYSTEM_NOT_READY followed by an inserted event. You need to draw the state machine on paper first to consider all the events at each state. You need to document the state machine diagram first to consider all the events at each state.

For the implementation part, you could design you state transition code in standalone function, and achieve 'control retry timespan and times' or 'local variables'.

In reply to: 310075723 [](ancestors = 310075723)

Hi Qi,
I understand the advantage of state machine approach. I just feel that the logic of xcvrd daemon is relatively simple and it seems that using state machine is less necessary.
IMO, state machine is adopted in event handling systems which have a lot of state and event, like network protocols. In those systems using state machine makes the flow clear and relatively easy to understand and also benefits the formal verification of that kind of system.
But for systems with less states and events the state machine approach doesn't have overwhelming advantage, since the current logic is straightforward. I have considered the state machine approach and found that only 2 states and 4 events. In this sense it's less necessary to go in this way.
meanwhile, I've created a PR in which I've updated the handling logic in a state-machine-like way, I think it is better than the current implementation. but it's not a strictly-speaking state machine. the pr is sonic-platform-daemons PR#5. You can check it.

for key, value in port_dict.iteritems(): [](start = 12, length = 40)

Why you put the for-loop outside the event/state branches? Can you move it inside a smaller branch? #Closed

Yes. You are correct.
Only for NORMAL_EVENT is this for loop required.
The "break" statements which originally break the inner "for" loop are also removed.

'INIT STATE' [](start = 13, length = 12)

You can use integer as value, don't rely on python to optimize the string comparison. #Closed

I think to use string is better since it makes logging information more human-readable.
Even though a dictionary can be adopted to map from integer value to string, the case that the state is not in the key set of the dictionary has to be handled. That's too complicated.
BTW, I'm not sure whether to use string address to compare the string is part of python's language specification or just optimization that depends on python interpreter's implementation?

states have been updated to integer value.
The events, like SYSTEM_FAIL, are already used by the plugins. In this sence, we'd better not to change them. It seems that to use string has already been a common practice in plugins, like the normal plugin/out events which are represented by '1', '0' respectively.

next_state [](start = 12, length = 10)

You may initialize it with state, next_state will never be None. #Closed

You can remove unexpected_state. This is redundant logic. #Closed

This is only for log.
By setting unexpected_state to true we can know why the state machine reach STATE_EXIT.

This is only for log.
By setting unexpected_state to true we can know why the state machine reach STATE_EXIT.

I decide to remove this. It is very unlikely to enter an unexpected state since all states are handled for each event.

repeated code, you can extract function. #Closed

now, only next_state is set when facing unexpected state. in this sense, no need to extract function.

You can move this part into event handling branches. #Closed

It's better to set it here since it can ensure that wherever the state reaches STATE_NORMAL the timeout can be properly set.

if not next_state == state: [](start = 12, length = 27)

For readability, use if next_state != state

fixed.