search

snoplus / tellie

The timing ELLIE LED calibration system
0 stars 11 forks source link

Code cleanup - switch to central parameters file #31

Closed mnirkko closed 7 years ago

mnirkko commented 7 years ago

A single file 'tellie.cfg' now sets all the necessary parameters to avoid redundancy. The parameters are read out by 'parameters.py' using the package ConfigParser, all scripts that import parameters can use them. Major change, successfully tested at Sussex test box. Log attached: testing_2017_03_17.txt

EdLeming commented 7 years ago

So I've started to look through this, but before I get further I think we should think about stream lining the tellie server code. Rather than copy / pasting the whole serial_command.py code, we should simply import the class and load it into the server - making keeping track of the code significantly easier.

Could you try to do so and run some tests? This would be a significant improvement and I think it's one we really should make.

mnirkko commented 7 years ago

Thanks Ed. I've already started commenting out duplicate functions in the server code that were clearly the same. For an entire class like SerialCommand, that may be more tricky. In the worst case I'll just do a "server" flag (boolean) that does X if true and Y if false. That can certainly be in one place, and it beats preprocessing flags. ;)

EdLeming commented 7 years ago

OK, so I was thinking about this on my walk in this morning. I think the solution is to simply delete serial_command.py. We can import everything all the test scripts need from the server instead (as the class is duplicated there). What do you think?

mnirkko commented 7 years ago

Yeah, that sounds doable. I'm worried it might make the tellie_server.py a bit heavy, but probably not much more than it already is (currently both scripts at 900 lines). It's worth a try.

On 21/03/17 13:25, EdLeming wrote:

OK, so I was thinking about this on my walk in this morning. I think the solution is to simply delete serial_command.py. We can import everything all the test scripts need from the server instead (as the class is duplicated there). What do you think?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/snoplus/tellie/pull/31#issuecomment-288076975, or mute the thread https://github.com/notifications/unsubscribe-auth/AGnvXG9ayxPgR9M8TnXyhcVjm8cPi4F5ks5rn8_VgaJpZM4MhK4K.

EdLeming commented 7 years ago

Hey, @mnirkko. I think this looks good! One thing to consider really is the size of these changes. Are we certain we have tested to the point that we're happy that this can be run underground?

mnirkko commented 7 years ago

I understand this point well. However, I am not certain how to test it better than checking the output underground and seeing whether we're happy with it. At least the parameters file will need some minor changes (select correct USB port + log file locations), but apart from that I "expect" it to work (didn't change the juicy parts, just moved them around).

On 24/03/17 12:54, EdLeming wrote:

Hey, @mnirkko https://github.com/mnirkko. I think this looks good! One thing to consider really is the size of these changes. Are we certain we have tested to the point that we're happy that this can be run underground?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/snoplus/tellie/pull/31#issuecomment-289014941, or mute the thread https://github.com/notifications/unsubscribe-auth/AGnvXNjK_Ia56SNZLCS7pbZOGJ20R4EGks5ro7zpgaJpZM4MhK4K.