opened
lee-matthews
username_0
Lee matthews/issue103 controller module
issue
created
jlebar
username_1
The new code should also go in lib/core if it can.  platformio can't test code outside of a library, so eventually all of our code than `main()` should be in a library.  I'm moving more code over in one of my outstanding PRs.
comment
created
lee-matthews
username_0
Ok, good point, go ahead
comment
created
lee-matthews
username_0
Normally, if the GUI sends the commands in the correct defined order, there
should never be the need for an error. But yeah, I do think this it is a
good idea to add one, the question I ask myself is how verbose the error
handling should be in terms of communications with the GUI device. I think
a lot is going to depend on the communications link architecture which is
currently in flux.
comment
created
asmodai27
username_2
If this is handled by the GUI, a generic "invalid command" should be enough.
comment
created
asmodai27
username_2
Made it so this PR works with new comms architecture. I also factored in some of the functions in respiration_ctrl in order not to break main.cpp controller loop.
Had to merge for this, rebase wouldn't do the trick with such a major change in master, sorry for the reviewers...
comment
created
jlebar
username_1
Hi, nice to meet you, and thanks for resurrecting this.  Since you've successfully done the work of merging it, the branch can probably now be squash-rebased onto master; and Eugene and I have enough git expertise to probably wrangle that.

I think this is probably not the refactoring we need at the moment.  Instead I think it's more important to work on testability of the current infrastructure, e.g. with #174, and maybe also separating out the inner PID loop from the rest of what's happening here.  I think we also should change how HAL works so that our calls into it are at a slightly higher level, "get me the X sensor reading" rather than "get me the sensor reading for pin Y".

I'm hoping to get some of this done today.
comment
created
asmodai27
username_2
Indeed, this is probably why Lee removed all of the actual PID in his original PR... will figure something out after sorting my history rewrite PR... Sorry again.
comment
created
jlebar
username_1
I think we can actually close this; with the refactoring that's going on, it's probably easier to reimplement this (I'm working on it rn).
comment
created
asmodai27
username_2
Yeah I realised this just now too, after trying to make it work with the current architecture once more...
comment