-
Notifications
You must be signed in to change notification settings - Fork 25
Enable Direct Browsing of LittleFS Storage Device on Windows #67
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
Conversation
src/littlefs/context.py
Outdated
|
|
||
|
|
||
| # if the user has win32file installed then define this class | ||
| try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try to avoid large try/excepts, as you could catch unintended exceptions. I would:
import ctypes
try:
import win32file
except ImportError:
pass
else:
# Put the UserContextWinDisk implementation here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I want to do! Didn't know I could do that. Also, instead of pass, I think it might be better to add a message to warn the user.
src/littlefs/context.py
Outdated
| def __init__(self, disk_path:str) -> None: | ||
| self.device = win32file.CreateFile(disk_path, win32file.GENERIC_READ, win32file.FILE_SHARE_READ, None, win32file.OPEN_EXISTING, 0, None) | ||
| if self.device == win32file.INVALID_HANDLE_VALUE: | ||
| raise Exception("Error opening disk") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a OSError or IOError might be more appropriate.
src/littlefs/context.py
Outdated
| try: | ||
| import win32file | ||
| except ImportError: | ||
| raise ImportError("Unable to import 'win32file'. This module is required for Windows-specific functionality. Please ensure you are running on a Windows platform or install 'pywin32' using: 'pip install pywin32'.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so hold on, this now makes win32file a hard requirement. We should only raise an error if the user attempts to instantiate UserContextWinDisk. The following is probably closer to what we want:
try:
import win32file
except ImportError:
win32file = None
class UserContextWinDisk(UserContext):
def __init__(self, disk_path:str) -> None:
if win32file is None:
raise ImportError(...)
# rest of WinDisk implementation
src/littlefs/context.py
Outdated
| import win32file | ||
| except ImportError: | ||
| win32file = None | ||
| else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so sorry, with this new implementation (checking win32file in __init__), then we no longer need this else:
try:
import win32file
except ImportError:
win32file = None
class UserContextWinDisk(UserContext):
...
Otherwise right now we get ImportErrors if win32file is not available.
BrianPugh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great to me! thank you!
This is an excellent Python interface for LittleFS; however, there is currently no support for users to directly browse the storage device with LittleFS on Windows. This pull request aims to address this by introducing support for browsing through the storage device. This is achieved by implementing a new UserContext with a device/file path arrangement.
Testing has only been conducted manually using an SD card adapter and USB flash.
A small sample code to show how it works.