Conversation
c95baf4 to
b59f300
Compare
There was a problem hiding this comment.
Thanks, this looks like a solid foundation.
Sorry I've put a lot of comments in the code; most of them are small nit-picks.
I managed to do a very small functional review but something has now bricked the board...?
Architectural
As discussed in-person, I think it would also be worth prototyping an action-queue based design for "updates" (from e.g. UDP, kafka, camonitor) which trigger the side-effects (changes to the Data object or adding more "updates" onto the queue). We should try it together, but it might give us an easier to reason about threading model - which might sidestep some of the more detailed threading/concurrency comments I've put elsewhere.
Troubleshooting / docs
At one point I think the FPGA went bad on me, got a crash like:
INFO:kafka_dae_control.static_pvs:begin
ERROR:kafka_dae_control.run_state:Failed to start run:
Traceback (most recent call last):
File "/home/ds/kafka_dae_control/src/kafka_dae_control/run_state.py", line 83, in on_run_state_change
write_verify(
File "/home/ds/kafka_dae_control/src/kafka_dae_control/comms.py", line 54, in write_verify
raise OSError(
OSError: (192.168.1.250) Could not write 1 32 bit words to address 0 (set data=19, readback=0)
INFO:kafka_dae_control.static_pvs:end
ERROR:kafka_dae_control.run_state:Failed to end run:
Traceback (most recent call last):
File "/home/ds/kafka_dae_control/src/kafka_dae_control/run_state.py", line 114, in on_run_state_change
write_and_inv_then_verify(
File "/home/ds/kafka_dae_control/src/kafka_dae_control/comms.py", line 89, in write_and_inv_then_verify
write_verify(sock, host, address, new_value, count, verify)
File "/home/ds/kafka_dae_control/src/kafka_dae_control/comms.py", line 54, in write_verify
raise OSError(
OSError: (192.168.1.250) Could not write 1 32 bit words to address 0 (set data=50, readback=51)
Can we start some troubleshooting docs about this?
In general - I think we need some "high-level" docs about what the assumptions/preconditions etc of this program are.
Functional
If I end the same run multiple times, I get multiple RunStops in Kafka - is that what we want? Should it check the runstate and ignore if already in SETUP?
| @@ -0,0 +1,166 @@ | |||
| """Utilities for communicating to a UDP device such as a streaming control board.""" | |||
There was a problem hiding this comment.
Please can you add some documentation about how you guarantee reads/writes in this module don't interleave with each other when called from multiple threads, or whether that's the responsibility of the caller (I think it should probably be the responsibility of this module).
Conceptually I think you likely need a (shared) lock at the top level around each function for this to be threadsafe.
But whichever approach we choose - whether it's a lock or a queue or something else, we need to document exactly what the behaviour is if these functions are called from multiple threads concurrently.
| Returns: None | ||
|
|
||
| """ | ||
| with state_file.open("w", encoding="utf-8") as file: |
There was a problem hiding this comment.
This seems like it probably needs a lock around it to prevent multiple threads from potentially calling it concurrently
Co-authored-by: Tom Willemsen <tomwillemsen1995@gmail.com>
Co-authored-by: Tom Willemsen <tomwillemsen1995@gmail.com>
Co-authored-by: Tom Willemsen <tomwillemsen1995@gmail.com>
Closes ISISComputingGroup/DataStreaming#24
This adds an architectural skeleton which provides basic functionality ie. the ability to begin and end a run (and to see whether the hardware is running) to
kafka_dae_controlBeyond this I have added several tickets to flesh the IOC out.
Notably I am going to leave the OPI for ISISComputingGroup/DataStreaming#25 to complete as currently the only thing that you can actually do is begin/end a run.