Skip to content
This repository was archived by the owner on Jun 13, 2023. It is now read-only.

Conversation

@mly32
Copy link
Contributor

@mly32 mly32 commented Dec 3, 2021

Summary

This PR addresses Jira ticket CISLUNAR-91

Tests running op-nav as a separate thread, with integration to the database (database as a queue), and also ensure the main satellite is not running during camera operation.

Testing

setup like so:

cd _todo_project_name
python -m venv venv
source ./venv/bin/activate
pip install -e ."[dev]"
python todo_project_name/main.py # main test

make help # possible commands
make docs # read through docs


# currently runs through a call of op-nav
# notice how there is no logging from main-sat during camera recording
# ... tests do not include att-adjust :(
# tests also do not auto enqueue elements onto op-nav run queue

Notes

Keeping everything separate right now. Just makes testing easier :\

Comments

Figured out a way to not need a separate .flake8 file so I updated pre-commit (Idk if you need to recall pre-commit install). Moved the opnav flight mode out of the flight_mode.py file. Hopefully, I didn't break anything while moving it out. It's not like I changed anything though... All major changes are in the "todo" project.

# do not run if global_lock is locked
with globals.run_camera_cond:
while globals.want_to_run_camera:
logging.info("Can to run camera set to TRUE")
Copy link
Member

@sjz38 sjz38 Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just be "Can run camera set to TRUE"? (no "to"?)

@sjz38
Copy link
Member

sjz38 commented Dec 3, 2021

So you essentially created another project within our project. Is the _todo_project_name structure what would be ideal for flight software?

@sjz38
Copy link
Member

sjz38 commented Dec 3, 2021

I ran it and it's a great start! I'll need to spend a little more time looking at it, but regardless, I don't think we'll be merging until we also have it working with real flight software. Thoughts, Toby?

@sjz38 sjz38 requested a review from tmf97 December 3, 2021 21:15
@sjz38
Copy link
Member

sjz38 commented Dec 3, 2021

(main-sat ) 16:16:00: Next adjustment too far away; time diff: 11.792185
I guess its fine that this outputs a value when running because the att-adjust queue is empty (so it won't transition to att-adjust mode)

@mly32
Copy link
Contributor Author

mly32 commented Dec 3, 2021

Yeah, it's pretty standard to have all the code in a separate folder. I wanted to restructure the main project, but that obviously isn't going to happen during the school semester when many people are making code changes.

So you have

- readme
- license
- requirements
- docs
- package-name
  - package-module
  - ...
- tests

Or something. A plus is that all the development code is conglomerated in one place. It makes pathfinding for documentation and testing a lot simpler.

Note that the att-adjust mode isn't doing anything right now. However, it made testing camera mode a lot easier.

With the use of condition variables, the thread won't be busy waiting (I believe).

By the way, you can see with the implementation of threading.Timer that during the database add op-nav run function, if you instead instantiated a Timer to run at a time x, it would altogether avoid the need for an op-nav run flight mode. A function could just check the database during reboots and update the queue/schedule.

Similar stuff can be done for maneuver and attitude adjustment. And radio would be run similarly on a scheduled thread that calls every `y' seconds.

@tmf97
Copy link
Member

tmf97 commented Dec 4, 2021

Off first glance, I really really like the new structure and would be extremely happy to overhaul our current implementation with this new framework.

@tmf97
Copy link
Member

tmf97 commented Dec 4, 2021

For purposes to clean software engineering, I think we should make a dev branch where all we do is "fill out" this new framework with what we have. Then once that works and has been tested, we can merge into master. Only working code should live on master.

I'm a fan of the package organization for purposes of presenting it on github, but while actually coding the extra folder might be a tad annoying (but worth the cleanliness).

I haven't dug into this too deeply, but what does this new structure give us that we didn't/couldn't have before? I only wanna completely overhaul FSW if there's a good reason to (I'm sure you have one, I'm just not sure what it is).

@mly32
Copy link
Contributor Author

mly32 commented Dec 4, 2021

I think functionality-wise, the project operates perfectly fine as is. But I was having trouble getting sphinx to document our project. If we think of cislunar as a python package, then I think the structure needs to be like so? for other tools to know where the code is easily (like publishing to PyPI? I could be totally wrong). I haven't been able to find a big project (numpy, pyright, black, etc.) that isn't structured this way.

Also, for documentation, reStructuredText is really expressive. Latex is built-in to its syntax as a math block :math:. There's ways to get diagrams but I haven't played around with it too much. We can always generate the diagrams in a separate latex document and import the image into reStructuredText.

@mly32
Copy link
Contributor Author

mly32 commented Dec 4, 2021

Link for sessions and theads: https://docs.sqlalchemy.org/en/13/orm/session_basics.html#is-the-session-thread-safe

Quote:

Making sure the Session is only used in a single concurrent thread at a time is called a “share nothing” approach to concurrency. But actually, not sharing the Session implies a more significant pattern; it means not just the Session object itself, but also all objects that are associated with that Session, must be kept within the scope of a single concurrent thread. The set of mapped objects associated with a Session are essentially proxies for data within database rows accessed over a database connection, and so just like the Session itself, the whole set of objects is really just a large-scale proxy for a database connection (or connections). Ultimately, it’s mostly the DBAPI connection itself that we’re keeping away from concurrent access; but since the Session and all the objects associated with it are all proxies for that DBAPI connection, the entire graph is essentially not safe for concurrent access.

It might be fine for one session per thread, but if two threads modify the same data,

If there are in fact multiple threads participating in the same task, then you may consider sharing the session and its objects between those threads; however, in this extremely unusual scenario the application would need to ensure that a proper locking scheme is implemented so that there isn’t concurrent access to the Session or its state. A more common approach to this situation is to maintain a single Session per concurrent thread, but to instead copy objects from one Session to another, often using the Session.merge() method to copy the state of an object into a new object local to a different Session.

@mly32
Copy link
Contributor Author

mly32 commented Dec 4, 2021

PyRight was failing because the "root" location for the project is FlightSoftware and not todo_project_name. It was giving warnings about modules like uitls.log and utils.constants not having defined vars. PyRight checks will need to be run on _todo_project_name later.

@sjz38 sjz38 added the opnav Issues related to optical navigation label Jun 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

opnav Issues related to optical navigation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants