Python stuff #19

fergusL · 2019-07-17T04:36:39Z

Just creating a draft pull request for now, will be adding more documentation/actual tests in the mean time. At the moment you won't really be able to test anything without a pi and automationhat.

f

was using bitwise operations to update the binary int that sets which leds are on/off. Couldn't find a bitwise method to create a turn_off() that would behave as intended if called more than once. switched to just converting the binary int to a string and then a list, then updating the list by a dictionary that maps the led name to its index in the string/list. Finally convert the list to a string and back to a binary int.

lspitler · 2019-07-30T22:37:44Z

OK @fergusL had another go. Can you turn on review on this page when you're ready for one and assign to whomever you'd like a review?

I added some initial notes in the README about installing relevant files on OSX. I also moved a chunk of text as I didn't think it related to Getting Started. Feel free to revert. Turned off warnings too - hard to debug with them.

I attempted a compilation, but it failed. Do you have any ideas what are missing from my setup? Error messages follow.

(huntsman) ➜  protos git:(python-stuff) ✗ make
g++ -march=core2 -mtune=haswell -mssse3 -ftree-vectorize -fPIC -fPIE -fstack-protector-strong -O2 -pipe -stdlib=libc++ -fvisibility-inlines-hidden -std=c++14 -fmessage-length=0 -std=c++11 -fPIC -O2 -g -DSB_LINUX_BUILD -I. -I./src/licensedinterfaces/ -I/usr/local/include -L/usr/local/lib -lprotobuf -pthread -lgrpc++ -pthread   -c -o src/main.o src/main.cpp
clang: warning: -lprotobuf: 'linker' input unused [-Wunused-command-line-argument]
clang: warning: -lgrpc++: 'linker' input unused [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-L/usr/local/lib' [-Wunused-command-line-argument]
In file included from src/main.cpp:4:
In file included from src/x2dome.h:6:
src/hx2dome.grpc.pb.h:27:7: error: definition of type 'CompletionQueue' conflicts with typedef of the same name
class CompletionQueue;
      ^
/usr/local/include/grpcpp/impl/codegen/completion_queue.h:26:38: note: 'CompletionQueue' declared here
typedef ::grpc_impl::CompletionQueue CompletionQueue;
                                     ^
In file included from src/main.cpp:4:
In file included from src/x2dome.h:6:
src/hx2dome.grpc.pb.h:28:7: error: definition of type 'Channel' conflicts with typedef of the same name
class Channel;
      ^
/usr/local/include/grpcpp/channel.h:26:30: note: 'Channel' declared here
typedef ::grpc_impl::Channel Channel;
                             ^
In file included from src/main.cpp:4:
In file included from src/x2dome.h:6:
src/hx2dome.grpc.pb.h:29:7: error: definition of type 'ServerCompletionQueue' conflicts with typedef of the same name
class ServerCompletionQueue;
      ^
/usr/local/include/grpcpp/impl/codegen/completion_queue.h:27:44: note: 'ServerCompletionQueue' declared here
typedef ::grpc_impl::ServerCompletionQueue ServerCompletionQueue;
                                           ^
In file included from src/main.cpp:4:
In file included from src/x2dome.h:6:
src/hx2dome.grpc.pb.h:30:7: error: definition of type 'ServerContext' conflicts with typedef of the same name
class ServerContext;
      ^
/usr/local/include/grpcpp/impl/codegen/server_context.h:25:36: note: 'ServerContext' declared here
typedef ::grpc_impl::ServerContext ServerContext;
                                   ^
4 errors generated.
make: *** [src/main.o] Error 1

lspitler · 2019-07-30T22:49:29Z

Oh, and can you let me know exactly what you'd like me to review? There is a lot here to parse.

fergusL · 2019-07-31T03:37:10Z

bad idea in hindsight, but when I started this branch for the raspberry pi python code I split off from the branch I had for the c++ driver.

The code that was intended to be reviewed for this PR is contained in huntsman-dome/domehunter/__init__.py. There is also an example jupyter notebook in huntsman-dome/examples/ that illustrates how the code works.

For this PR don't worry about any of the gRPC/compiling stuff. I was just assuming that that PR would have closed by the time I'd be submitting this one, so there wouldn't be duplication of commits across two PRs.

This PR was meant to be a general assessment of how the dome control is set up and whether this is a decent base or if significant changes need to be made before we start worrying about fail safes/how the code might break during operation.

fix2

lspitler

OK @fergusL heroic job on this! Well done with clearly written code and logic that I can see makes sense.

Fine for you to merge after addressing each of my suggestions.

Can we discuss this one in person:
"OK, I don't get the need to have encoder_pin and encoder in testing mode and normal mode, respectively. Just makes it confusing. Ditto for home_sensor_pin and home_sensor. Let's chat about this one."

lspitler · 2019-08-05T21:41:17Z

README.rst

@@ -26,31 +26,26 @@ and tracks the dome position using an encoder. It returns infomation
 The c++ code is built around Software Bisque's X2 standard. For more
 infomation on this `see here <https://www.bisque.com/x2standard/class_x2_dome.html#a7ffd792950cdd0abe1b022e7a8caff9e>`.


Suggested change

infomation on this `see here <https://www.bisque.com/x2standard/class_x2_dome.html#a7ffd792950cdd0abe1b022e7a8caff9e>`.

infomation on this `see here <https://www.bisque.com/x2standard/class_x2_dome.html#a7ffd792950cdd0abe1b022e7a8caff9e>`_.

lspitler · 2019-08-05T21:41:48Z

README.rst

@@ -26,31 +26,26 @@ and tracks the dome position using an encoder. It returns infomation
 The c++ code is built around Software Bisque's X2 standard. For more


Suggested change

The c++ code is built around Software Bisque's X2 standard. For more

The C++ code is built around Software Bisque's X2 standard. For more

lspitler · 2019-08-05T21:49:03Z

README.rst

+  brew install grpc
+
+Getting Started
+---------------

 The files for compilation and installation are found in the


I can't give a suggestion on the wording below, but maybe move "In order to compile the driver simply run the makefile recipe." before you state "You should be able to simply run the shell script" so someone can just follow the flow in order.

Also good to have a clear set of commands:

cd domehunter/protos/ make sh TheSkyX_plugin_install.sh

lspitler · 2019-08-05T21:53:12Z

README.rst

@@ -90,8 +85,48 @@ adjusted to your local machine. You shouldn't need to worry about
 this as the generated files are committed to the repositry and
 shouldn't need to be generated.


Suggested change

shouldn't need to be generated.

shouldn't need to be generated. These files were used in the previous "Getting Started" step to create the `libHuntsmanDome.so` file, so no further editing of anything below is needed unless you need to XXX.

lspitler · 2019-08-05T21:56:59Z

README.rst

+* ``hx2dome.proto`` - language agnostic RPC definitions used by everthing
+* ``hx2dome.proto_server.py`` - python server that receives RPC from TSX
+
+The remaining cpp and python files are automatically produced


Should this be moved somewhere else? It references its own section.

lspitler · 2019-08-05T23:39:42Z

domehunter/__init__.py

+    def _turn_led_off(self, leds=[]):
+        """Method of turning a set of debugging LEDs off"""
+        # pass a list of strings of the leds to turn off
+        if self.debug_lights:


Suggested change

if self.debug_lights:

if (self.debug_lights):

return None

lspitler · 2019-08-05T23:41:19Z

domehunter/protos/Makefile

@@ -1,10 +1,10 @@
 # Makefile for libHuntsmanDome

 CXX = g++
-CFLAGS = -fPIC -Wall -Wextra -O2 -g -DSB_LINUX_BUILD -I. -I./src/licensedinterfaces/\
+CFLAGS = -fPIC  -O2 -g -DSB_LINUX_BUILD -I. -I./src/licensedinterfaces/\


I see a LINUX thing here. Maybe need a Makefile_linux and Makefile_osx as related to this issue: #25

lspitler · 2019-08-05T23:41:44Z

requirements.txt

@@ -1,2 +1,4 @@
 astropy_helpers
 gpiozero
+smbus


Suggested change

smbus

smbus # optional for LED use

lspitler · 2019-08-05T23:41:56Z

requirements.txt

@@ -1,2 +1,4 @@
 astropy_helpers
 gpiozero
+smbus
+sn3218


Suggested change

sn3218

sn3218 # optional for LED use

lspitler · 2019-08-05T23:42:24Z

setup.cfg

@@ -43,4 +43,4 @@ install_requires = astropy
 # version should be PEP440 compatible (https://www.python.org/dev/peps/pep-0440/)
 version = 0.0.dev
 # Note: you will also need to change this in your package's __init__.py
-minimum_python_version = 3.7
+minimum_python_version = 3.6


Do we need a max python version for the Pi? Or just remove these again if we don't really need astropy

lspitler

OK @fergusL heroic job on this! Well done with clearly written code and logic that I can see makes sense.

Fine for you to merge after addressing each of my suggestions.

Can we discuss this one in person:
"OK, I don't get the need to have encoder_pin and encoder in testing mode and normal mode, respectively. Just makes it confusing. Ditto for home_sensor_pin and home_sensor. Let's chat about this one."

lspitler · 2019-08-09T01:52:43Z

domehunter/__init__.py

+                if self.testing:
+                    # if testing simulate a tick for every cycle of while loop
+                    self._simulate_ticks(num_ticks=1)
+                pass


see comment below about sleeping

lspitler · 2019-08-09T01:57:04Z

domehunter/__init__.py

-        raise NotImplementedError
+    def calibrate(self, num_cal_rotations=2):
+        """Calibrate the encoder (determine degrees per tick)"""
+        # rotate the dome until we hit home, to give reference point


As @AnthonyHorton suggested, break part of the code below into a goHome method.

fergusL and others added 10 commits July 16, 2019 02:04

rough implementation

d3899d9

added a few comments/autopep8

38d3da7

added a testing mode

1f394a7

added debug light mode

4d26963

f

import error handling

19e615c

comments/readme/requirements

41415f3

formatting error

b57e5a3

formatting error2

0d86e2e

osx install, restructure

4029c2d

turn off all compilation warnings

a7b752d

fergusL added 2 commits July 31, 2019 14:19

fix small formatting error

fdb0363

fix2

372c580

fix2

fergusL marked this pull request as ready for review July 31, 2019 04:45

fergusL requested review from lspitler, wtgee and AnthonyHorton July 31, 2019 04:45

Merge branch 'develop' into python-stuff

ea7607f

lspitler approved these changes Aug 5, 2019

View reviewed changes

lspitler reviewed Aug 9, 2019

View reviewed changes

fergusL and others added 5 commits August 16, 2019 03:35

tidy up

68eea59

tweaks to make/install files

c410ad6

update

4081e4d

tidy up

ced51de

tidy2

1549ee6

fergusL merged commit d8bb0a7 into AstroHuntsman:develop Aug 27, 2019

fergusL deleted the python-stuff branch October 10, 2019 04:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python stuff #19

Python stuff #19

fergusL commented Jul 17, 2019

lspitler commented Jul 30, 2019

lspitler commented Jul 30, 2019

fergusL commented Jul 31, 2019 •

edited

Loading

lspitler left a comment

lspitler Aug 5, 2019

lspitler Aug 5, 2019

lspitler Aug 5, 2019

lspitler Aug 5, 2019

lspitler Aug 5, 2019

lspitler Aug 5, 2019

lspitler Aug 5, 2019

lspitler Aug 5, 2019

lspitler Aug 5, 2019

lspitler Aug 5, 2019

lspitler left a comment

lspitler Aug 9, 2019

lspitler Aug 9, 2019

		@@ -26,31 +26,26 @@ and tracks the dome position using an encoder. It returns infomation
		The c++ code is built around Software Bisque's X2 standard. For more
		infomation on this `see here <https://www.bisque.com/x2standard/class_x2_dome.html#a7ffd792950cdd0abe1b022e7a8caff9e>`.

		@@ -90,8 +85,48 @@ adjusted to your local machine. You shouldn't need to worry about
		this as the generated files are committed to the repositry and
		shouldn't need to be generated.

	shouldn't need to be generated.
	shouldn't need to be generated. These files were used in the previous "Getting Started" step to create the `libHuntsmanDome.so` file, so no further editing of anything below is needed unless you need to XXX.

Python stuff #19

Python stuff #19

Conversation

fergusL commented Jul 17, 2019

lspitler commented Jul 30, 2019

lspitler commented Jul 30, 2019

fergusL commented Jul 31, 2019 • edited Loading

lspitler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lspitler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fergusL commented Jul 31, 2019 •

edited

Loading