Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes to hardware initialization #96

Merged
merged 6 commits into from
Jan 21, 2025
Merged

Changes to hardware initialization #96

merged 6 commits into from
Jan 21, 2025

Conversation

dbgen1
Copy link
Contributor

@dbgen1 dbgen1 commented Jan 17, 2025

Hardware initialization occurs in pysquared.py, where all the devices are initialized. Many of these devices the same behavior when it comes to their initialization, so it is beneficial to write functions that can handle initialization behavior. This PR does a couple things:

  1. Add a init_general_hardware function which is able to handle the hardware initialization for any device that simply needs a firmware constructor. To keep the current code behavior unchanged, this function also optionally takes a callable which modifies the initialization behavior if self.orpheus is true. A detailed docstring in this function describes its behavior in more detail.
  2. Moves the initialization of hardware devices into their own functions, which are called in the constructor of Satellite. Using separate single-use functions for implementation makes the code self-documenting and allows for easier testability as per (3).
  3. Add a @safe_init decorator to the Satellite class which wraps all the initialization functions in the standard try catch used for every hardware instance with a defined error severity if needed, and can print detailed debug information if self.debug is enabled.

@Mikefly123
Copy link
Member

Hey @dbgen1! The proposed changes sound like they could be pretty great. Particularly, the creation of individual functions for hardware initialization seems like a good abstraction to increase code readability and also probably allow us to individually deinit and reinit individual pieces of hardware more easily.

I had a separate question though about this PR, it looks like there are a bunch of files that are "changed" but are identical to the origin files. Not super familiar with why this happens sometimes with GitHub. Is it a file formatting issue?

@dbgen1
Copy link
Contributor Author

dbgen1 commented Jan 18, 2025

I'm not sure why it happens. I noticed it as well and used a diff tool to verify that nothing important was actually changed. It might have to do with the way I cloned the repo to my machine?

@Mikefly123
Copy link
Member

@nateinaction have you seen this happen before? I think it happened once or twice before to me on Mac. @dbgen1 are you using cloud syncing by any chance. I think this was happening to be when I was cloning my repos to a folder that was being backed up by my iCloud Drive and it went away once I started cloning to a folder that was not backed up by the iCloud.

I think I fixed the issue by removing and then recloning the repository.

Copy link
Member

@nateinaction nateinaction left a comment

Choose a reason for hiding this comment

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

Hmmm. This is pretty strange and I would think that the pre-commit hook would have caught this. It looks like an extra invisible char was added to every line by your editor. I suspect some delving into your editor config might solve this problem but you also might need to reapply your changes onto a new branch:

git diff origin/main --word-diff-regex=.
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
index db70d31..c1b8104 100644
--- a/.github/workflows/ci.yaml
+++ b/.github/workflows/ci.yaml
@@ -1,53 +1,53 @@
name: ci{+^M+}
{+^M+}
on:{+^M+}
  pull_request:{+^M+}
  push:{+^M+}
    branches:{+^M+}
      - main{+^M+}
{+^M+}
jobs:{+^M+}
  lint:{+^M+}
...

Even though I've seen the code in office hours and like the idea, the PR isn't easily reviewable in this state. Blocking.

@dbgen1 dbgen1 force-pushed the hardware_init_changes branch from 65b0dc8 to 14a1037 Compare January 19, 2025 00:13
@dbgen1
Copy link
Contributor Author

dbgen1 commented Jan 19, 2025

The issue was caused by the editor changing the whitespace at the end of each line from a Unix style (LF) line ending to a windows style (CRLF) line ending. I fixed this by configuring git to automatically convert the line ending style to the correct one used in this repository (LF). The latest commit contains all the actual changes and shows that there were no other modifications.

@dbgen1 dbgen1 requested a review from nateinaction January 19, 2025 00:18
Copy link
Member

@nateinaction nateinaction left a comment

Choose a reason for hiding this comment

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

Much better! Looks like there was an issue with resolving a merge conflict that removed #87. Once that's fixed I'll test this on my board and give you a 👍

lib/pysquared/pysquared.py Outdated Show resolved Hide resolved
lib/pysquared/pysquared.py Outdated Show resolved Hide resolved
@dbgen1
Copy link
Contributor Author

dbgen1 commented Jan 19, 2025

Going to do one more commit to appease the linter and this should be good to go!

@dbgen1 dbgen1 requested a review from nateinaction January 19, 2025 23:49
Comment on lines +143 to +144
if self.orpheus and orpheus_func:
return orpheus_func(hardware_key)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this stops the hardware from being initialized but does not stop the hardware from being set to an initialized state in the hardware dictionary.

Device  | Status
|I2C0   | True |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed this by moving the dictionary setting to each individual init function rather than the decorator, which is a method that makes more logical sense anyway.

@dbgen1 dbgen1 requested a review from nateinaction January 21, 2025 02:47
@dbgen1 dbgen1 merged commit 83d6635 into main Jan 21, 2025
3 checks passed
@dbgen1 dbgen1 deleted the hardware_init_changes branch January 21, 2025 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants