Software tasks to do after merging our branches (Done)

[hamishmb]

Patrick and I were discussing the software this morning, and it occurred to us that there are a number of tasks we might want to do, but that it will be hard to do until after we've merged all our branches together.

I've decided to start a list for us to keep things organised.

• Move some of the existing unit tests into Patrick's new modules (which ones?).
• Merge config.py - there's a lot of new config stuff for extra gate valves in master now - Done
• Control logic stuff - Done
  • Create rivercontrolsystem/Logic/controllogic.py ; somewhere for main.py to find control logic functions and control logic set-up functions - Done
  • Create rivercontrolsystem/Logic/stagepilogic.py ; control logic for Stage Pi - Done by Patrick
  • rivercontrolsystem/Logic/sumppilogic.py ; control logic for Sump Pi - Not needed as this is old-style logic
  • Create rivercontrolsystem/Logic/gatevalvelogic.py (shared between the gate valves, maybe except the Matrix pump) - Not yet needed, waiting until we have Matrix pump logic/hysteresis for gate valves
  • Update unit tests to match functions moved to these new files - Done
• Sockets stuff - Done
  • Clear out any duplicate tick requests when socket disconnects and reconnects - we only need one, no need to put extra load on the NAS box - Done
• Slightly modify and add the hall effect probe diagnostic code to master in case we need it again later - Done
• Update remaining out of date bits of software documentation - Done
• Rename master branch to "main" or similar (BLM). (Waiting for Hamish to try on his personal projects first in case it breaks everything ).

Before or after merging:

• Make sure Patrick's tests are integrated with the test script - Done
• Decide on "trigger" levels for Stage Pi - Do during/after deployment

Status of Patrick's code:

• Pretty much all the tests are complete.
• ^ It should be deployable at this stage.

[PatrickW]

After merging the branches, I'd like to create these files:
rivercontrolsystem/Logic/controllogic.py ; somewhere for main.py to find control logic functions and control logic set-up functions
rivercontrolsystem/Logic/stagepilogic.py ; control logic for Stage Pi
rivercontrolsystem/Logic/sumppilogic.py ; control logic for Sump Pi

Code that's useful to more than one piece of logic would go in Tools still. For example, I created rivercontrolsystem/Tools/statetools.py, which contains some classes which make it easier to write control logic that follows The State Pattern.

I suppose I could in theory create stagepilogic.py from the outset, since that's all new code, but I don't want to do that unless we're definitely doing this.

This all has implications for unit tests. Obviously the tests have to be updated to look in the right files, but it would also make sense to move the Sump Pi control logic tests out of the coretools test suite, to match the new location of the logic. I'm yet to write unit tests for the Stage Pi logic, so I was thinking it would make sense to write them in the intended new test suite instead of coretools. Again, don't want to do this unless we're definitely moving the code.

I'd also like to modify config.py main.py and/or coretools.py (can't remember exactly which bits are relevant) so that, as well as checking whether there is a control logic function defined for a Pi in the config, we also check whether there is a control logic setup function defined for the Pi. The setup function, if defined, would run once before the main loop starts. This is necessary because the Stage Pi logic will ideally have an object instance set up for it, to keep track of its current state. Need to decide the best way to get a reference to that object into the control logic function. (Create a dictionary and pass it to the set-up and logic functions, and put the object in there?)

[hamishmb]

Okay. I have a question

If we have controllogic.py, why would we also need stagepilogic.py and sumppilogic.py?

I suppose I could in theory create stagepilogic.py from the outset, since that's all new code, but I don't want to do that unless we're definitely doing this.

This all has implications for unit tests. Obviously the tests have to be updated to look in the right files, but it would also make sense to move the Sump Pi control logic tests out of the coretools test suite, to match the new location of the logic. I'm yet to write unit tests for the Stage Pi logic, so I was thinking it would make sense to write them in the intended new test suite instead of coretools. Again, don't want to do this unless we're definitely moving the code.

I reckon this is a good idea, I just don't understand the organisation yet

I'd also like to modify config.py main.py and/or coretools.py (can't remember exactly which bits are relevant) so that, as well as checking whether there is a control logic function defined for a Pi in the config, we also check whether there is a control logic setup function defined for the Pi. The setup function, if defined, would run once before the main loop starts. This is necessary because the Stage Pi logic will ideally have an object instance set up for it, to keep track of its current state. Need to decide the best way to get a reference to that object into the control logic function. (Create a dictionary and pass it to the set-up and logic functions, and put the object in there?)

I would make the object at the module level where the control logic function resides. For example, if your function is at Logic/sumppilogic.py, the setup function could (as long as Logic/sumppilogic.py is imported) create the object as Logic.sumppilogic.<somemeaningfulnamehere>. Then no passing around of the object is needed, and no chance of code in other modules accidentally changing it (due to a name conflict, say). Make sense?

[PatrickW]

If we have controllogic.py, why would we also need stagepilogic.py and sumppilogic.py?

I think it comes down to taste/code style. I could go along with just having the one file. Personally I think each Pi's logic is sufficiently lengthy to justify putting it in a separate file. I tend to err on the side of splitting things up, but opinions may differ on this. Your call. I'm not overly bothered one way or the other.

The reason I am suggesting controllogic.py in addition to stagepilogic.py and sumppilogic.py is just to avoid having to significantly change the way the control logic functions are 'found' by by main.py; i.e. to give it one module it can look in, rather than having to change it so that it locates a named module.

Did you notice I was suggesting to create a Logic directory? How do you feel about that? It probably doesn't make much sense if it only contains one file: might as well just put controllogic.py in the root in that case.

Need to decide the best way to get a reference to that object into the control logic function. (Create a dictionary and pass it to the set-up and logic functions, and put the object in there?)

I would make the object at the module level where the control logic function resides. For example, if your function is at Logic/sumppilogic.py, the setup function could (as long as Logic/sumppilogic.py is imported) create the object as Logic.sumppilogic.<somemeaningfulnamehere>. Then no passing around of the object is needed, and no chance of code in other modules accidentally changing it (due to a name conflict, say). Make sense?

That sounds like a much more sensible method. I never thought of doing it like that! I think my lack of experience in functional programming styles has probably blinded me to that possibility.

[hamishmb]

I think it comes down to taste/code style. I could go along with just having the one file. Personally I think each Pi's logic is sufficiently lengthy to justify putting it in a separate file. I tend to err on the side of splitting things up, but opinions may differ on this. Your call. I'm not overly bothered one way or the other.

The reason I am suggesting controllogic.py in addition to stagepilogic.py and sumppilogic.py is just to avoid having to significantly change the way the control logic functions are 'found' by by main.py; i.e. to give it one module it can look in, rather than having to change it so that it locates a named module.

Did you notice I was suggesting to create a Logic directory? How do you feel about that? It probably doesn't make much sense if it only contains one file: might as well just put controllogic.py in the root in that case.

Okay, this makes sense, let's do it the way you suggested. Note: How do you feel about the control logic functions getting the readings using the methods of CoreTools.DatabaseConnection, rather than the main loop doing it for them? This might reduce the coupling and make the main loop simpler I think. I'll add that stuff to the list.

That sounds like a much more sensible method. I never thought of doing it like that! I think my lack of experience in functional programming styles has probably blinded me to that possibility.

Didn't realise that counted as functional programming

I don't know much about that either, it's just the simplest way that occurred to me to do things.

EDIT: List updated.

[PatrickW]

How do you feel about the control logic functions getting the readings using the methods of CoreTools.DatabaseConnection, rather than the main loop doing it for them? This might reduce the coupling and make the main loop simpler I think.

I'm not sure I like that. It would decrease the coupling between main and the specific strategy for getting readings, but it would increase the coupling between the control logic and that strategy. Currently, the control logic functions don't have to know anything about how the readings are obtained, and I think it is better that way around. I think making the control logic depend on the specific means of obtaining readings would violate the dependency inversion principle. (Or something related to it.)

This kind relates to what I was saying before in the Software design for monitoring database readings thread about standardising the interface for the DatabaseConnection class. (Although I think I might have been making a slightly different point in that thread.) Currently run_standalone takes whichever method of fetching readings is being used and converts it into a readings dictionary, which constitutes a standardised interface. If run_standalone isn't going to abstract the readings to a standardised interface, then I think something else should.

[Cue lengthy-aside background music]

One way to remove the code from run_standalone without shifting the dependency and the coupling into the control logic would be to write an abstract ReadingsFetcher class (or choose a better name), which provides a get_readings method (Or getReadings? I've lost track of which style we are using. ) Then, a DatabaseReadingsFetcher (or choose a better name) can be written, which extends ReadingsFetcher and performs a similar job to the code you want to remove from run-standalone. Then all run_standalone would have to do is instantiate a DatabaseReadingsFetcher, as, e.g., controllogic.readings_fetcher. Then, the logic can just call controllogic.readings_fetcher.get_readings() without knowing where they come from, run_standalone doesn't need to know where they come from either, and it's fairly easy to write a different readings fetcher if it the way readings are fetched changes again in the future, and it's easy to switch back and forth between different ways of fetching readings.

If there are some features of the database that the control logic might want to use directly, but which wouldn't necessarily available if the readings are fetched by another means, then hopefully they can be added to the ReadingsFetcher abstraction in such a way that they return sensible data even if the underlying method of fetching the readings doesn't actually support them. In other words, the abstract class' interface should enable the logic to say "I'll give you an opportunity to try this, but if you can't do it, you can fall back on something else." So, just as an example, maybe the logic only wants a specific reading, and the DatabaseConnection can efficiently return just that reading, so we want to provide a method for doing that. If someone then writes a SocketsReadingsFetcher, though, that same method would have to get all of the readings and just filter out the one the logic wants. (Presumably it would store the readings in the Fetcher object, in case the logic wants one of the other readings.)

Or, a completely different way of doing it would be to just write a function that returns a readings dictionary and fill it with the code that would otherwise produce a readings dictionary in run_standalone. If the method of fetching the readings changes, write a new function. Less flexible, but conceptually simpler.

[End lengthy-aside background music]

How does that sound? Am I talking any sense?

Didn't realise that counted as functional programming

Functional is probably (definitely) the wrong word. What I really meant was a word that describes whatever various styles I'm not super familiar with!

Which is not to say that putting the object in the module namespace is functional, or whatever other style, it's just that I would tend to rely much more heavily on objects, to the point that there would be no need to get a reference to this object into the logic function, because instead of running a function we would be instantiating an object and running a method on that, and any variables/objects the method needed would be inside the object.

But I don't want to say we should do that, because I'm trying to adapt myself to the codebase rather than imposing the style I know onto it. I think it's good to get experience of different ways of doing things.

Putting variables in the module namespace is conceptually pretty darn similar to putting them inside an object.

[hamishmb]

Lots of good ideas

Or, a completely different way of doing it would be to just write a function that returns a readings dictionary and fill it with the code that would otherwise produce a readings dictionary in run_standalone. If the method of fetching the readings changes, write a new function. Less flexible, but conceptually simpler.

I prefer this idea I think, we can just have a function in each of the control logic files for that - it keeps the complexity out of the main loop and the main control logic function that way. Sound good?

[PatrickW]

I prefer this idea I think, we can just have a function in each of the control logic files for that - it keeps the complexity out of the main loop and the main control logic function that way. Sound good?

What about writing the function only once, and placing it either in Tools or in the shared controllogic.py?

I've made a Tools/statetools.py containing classes for building state machine based logic. Perhaps it would be good to change that into a Tools/logictools.py instead, which could contain the state tools but also the readings function(s).

I don't think we're on entirely the same page with regard to Logic and Tools. I've built up a fairly rigid definition about what goes where, and I have been wrongly assuming that you have the same definition, because I derived my definition from reading the existing codebase and documentation, so it felt like you and Terry were telling me the definition! Turns out, I must have been adding something of my own interpretation to the mix as well.

My thinking is that the logic itself is not part of the framework, but rather the thing that runs inside (or gets draped over) the framework. The logic is like a form of configuration that's too complicated to include directly in config.py. Hence my suggestion of a Logic directory for the logic, since Tools has been designated as containing the framework.

But things that help you write the logic, like state machine tools and readings-fetching functions, seem to me like things that should be part of the framework, or at least some kind of library, rather than part of the logic. They don't define the rules, they just provide simpler ways of expressing the rules.

How does this compare to your thinking?

[hamishmb]

This all seems reasonable.

I don't particularly like the idea of it being only one function, because each control logic function will need different readings anyway. We don't need all the pics to know about all of the other pis' readings (unnecessary and inefficient due to excessive database queries), so I'd rather have a function for each site/pi, rather than long if ladders in one enormous function.

It's entirely possible to have it in one function, but if we're having a set of files for logic-related functions anyway, why not split it into smaller, more specific functions for each site/pi, and define which one to call in config.py perhaps?

Maybe we're talking about cross purposes? I have a feeling I'm missing a detail here because I haven't seen the new logic - perhaps the single function idea makes more sense knowing how the new logic works.

[PatrickW]

Ah, OK, I think I see the mismatch here, and I think it's my fault for not being clear about what I mean.

I described a kind of compatibility layer that gives the logic a dictionary of readings, like the one it currently gets. I think that's what you're envisaging.

But I was taking it as read that it should be generic across the logic, as a point of principle. Any way of making it more efficient had to take account of this constraint. I kind of jumped on ahead and thought "yeah, that looks feasible", but didn't elaborate on it.

Meanwhile, I left you with my basic idea. You were thinking "well this isn't very efficient" and trying to poke it into shape by splitting it into a bunch functions that each return only the readings needed by a particular site's control logic. But I think that arrangement is daft. I imagine you think it's an odd idea too, but perhaps you are going along with it because you are trying to reach agreement with me. If we did it like that, we'd be taking specific readings, bundling them together, then the logic is just going to split them apart again. What a waste of time.

The value of providing a readings dictionary is only really as a drop-in replacement for the existing readings dictionary, which would be generic across all the logic. But if we end up having to write a whole bunch of site-specific functions that pair up with the site-specific control logic, those functions are effectively part of that site's control logic. At which point all we've really ended up with is a convoluted way for the control logic to deal with readings. And, I definitely wouldn't want one function that knows what readings each site's logic wants, with a massive if statement or whatever. That would just be a total mess.

When I was thinking ahead, I didn't go down that route. I thought: If the logic just wants specific readings, then what it really needs is a function that returns only the latest reading for a specific sensor at a specific site. That function call can be written into the logic wherever the logic currently accesses the readings dictionary. But this can't just be a method on DatabaseConnection. If we decide we need a DatabaseMonitor, or if some other arrangement crops up in the future, then the aim is to avoid having to change the control logic again. The logic should just be able to keep on calling the same function to get these readings.

When I said "If the method of fetching the readings changes, write a new function," my mind was in the mode of thinking about dependency injection. The logic would expect a function with a particular signature, but the identity of the function would be injected in, so that the logic does not need to know or care which function it is using. When I said that, I hadn't really thought about how to do that. But, I think you can assign a function to a variable in Python (first class functions), which would enable the logic to refer to the function by the name of the variable rather than by the name with which the function has been defined. If the function would end up being a straight-through wrapper around another function or method, then we could just assign that function or method to the variable directly, rather than writing a wrapper.

To summarise, I think the two details you are missing are:

1. I hadn't fully decided what arguments the function should take, and what its return value should be, but I'd run on ahead with a specific idea that I didn't tell you about. (Return readings for a specific site and sensor.)
2. I want to inject the function into the logic, rather than hard code it into the logic, so that it's easy to swap it for a different one later on.

Also, the logic may need more than one function like this, for different purposes (e.g. controlling devices). I'm only talking about fetching readings here.