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

Added rpi plugin for controlling components/sensors #93

Closed
wants to merge 1 commit into from

Conversation

44yu5h
Copy link

@44yu5h 44yu5h commented Apr 25, 2024

  • Created rpi plugin which only appears when running Raspberry Pi
  • Added support for
    • Digital input & output
    • All kinds of buttons
    • HCSR04 Distance sensor
    • All types of OLED displays
    • Some additional helpful blocks (high, low, delay)
  • Added all the dependencies for them
  • Readme on how to contribute further
  • All the libraries have an MIT License.

Do let me know if there is something to improve or add.

Also, I added to the system path to import local libraries (see here). Relative imports were somewhat troubling me and also it was feeling bulky to rewrite the path every time. But anyway, I may be wrong. Let me know if I can improve it.

Thanks

- create rpi plugin which only appears when running on Raspberry Pi
- add support for
	- digital input & output
	- all kinds of buttons
	- HCSR04 Distance sensor
	- all types of OLED displays
	- some additional blocks (high, low, delay) for side
- add all the dependencies for them
@quozl
Copy link
Contributor

quozl commented Apr 25, 2024

You're adding about 487 files. I looked briefly and saw a few licenses. Your commit message doesn't mention if you considered licensing. Could you improve that?

Also, you are adding a lot of Python modules deep inside the app. These won't be updated automatically, and will rapidly age. The Adafruit modules in particular change quite freequently.

@44yu5h
Copy link
Author

44yu5h commented Apr 25, 2024

You're adding about 487 files. I looked briefly and saw a few licenses. Your commit message doesn't mention if you considered licensing. Could you improve that?

Yes, they all have an MIT License, forgot to mention. My bad 👍🏻
The total size of plugin is 2.8MB excluding the screenshots.
Also, as suggested by @walterbender , I will remove some unused libraries ~200 files

Also, you are adding a lot of Python modules deep inside the app. These won't be updated automatically, and will rapidly age. The Adafruit modules in particular change quite freequently.

I thought about that but if you look at electronics, the sensors and modules don't change at all if I may say that. There are electronic components from 15-20 years ago still being used in large scales. The models change (that too rarely) and we will have to include support for them anyway, but if it works now then it will work forever for that model.
So I think if a library works on a model of a sensor then updating the library is not in the best interest. But anyway, we can have it both ways.

I don't have much experience with bundling libraries. So what I did was download the required prebuilt modules from pypi.org and put them in libs folder similar to as it is present in python3/site-packages.

Apart from the libs folder there really are only 7 files. So, I don't know, what should I do? Add a pip install command in docs to install modules, or achieve them in a zip or something else? Please do let me know.

This is the structure:

rpi (8 files)
├── rpi.py
├── functions.py
├── libs (~480 files, contains all the libraries)
│   └── //dependencies//
├── sensors
│   ├── add_path.py
│   └── //other sensor's scripts//
└── //some other files//

Thanks

@chimosky
Copy link
Member

chimosky commented Apr 25, 2024

I don't have much experience with bundling libraries. So what I did was download the required prebuilt modules from pypi.org and put them in libs folder similar to as it is present in python3/site-packages.

Apart from the libs folder there really are only 7 files. So, I don't know, what should I do? Add a pip install command in docs to install modules, or achieve them in a zip or something else? Let me know.

Adding a part to the readme that states how to install the packages is a good solution because this is a fructose activity and bundling it with dependencies means the size of Sugar increases in our downstream, and these dependencies are only needed when using sensors on RPi.

@44yu5h 44yu5h marked this pull request as draft April 25, 2024 15:12
@quozl
Copy link
Contributor

quozl commented Apr 25, 2024

Downstream packaging, such as flatpak, snap, or debian, can handle the dependencies automatically, we just have to make sure they are discoverable by those packaging processes. Having the plugin report missing dependency would be nice, rather than just failing to proceed.

@44yu5h
Copy link
Author

44yu5h commented Apr 27, 2024

Sorry, I was a little caught up with my exams.
I made a new branch with automatic installation of dependencies. You just have to run a TurtleArt block and everything gets set up. It was inspired by Devin's suggestion on Matrix. See this ReadMe. It's pretty simple now

Can you check this commit please before I create a PR? Let me know if you find any poor implementations. Sorry for the trouble. Thanks ^^
master...44yu5h:turtleart-activity:master


Adding a part to the readme that states how to install the packages is a good solution

@chimosky Thanks! The size is now ~100KB, without the dependencies


flatpak, snap, or debian, can handle the dependencies automatically

@quozl Thanks for letting me know about it. I don't have experience in any of those. Should I look into it?
I think that should be done when we create a separate version of sugar for RPi.


@walterbender Let me know if this is alright sir. Thanks

@walterbender
Copy link
Member

This approach seems to make sense.

One little thing: Some paths are pretty brittle. Maybe make them relative? e.g.

@quozl
Copy link
Contributor

quozl commented Apr 27, 2024

Reviewed 7da12b1

  • probe for what is needed before making a change; both with raspi-config and with imports, so that if a flatpak, snap, or debian packaging of the software satisfies the requirements your code ends up doing nothing,
  • add a clear list of dependencies to README.md, for use by the developers who do packaging,
  • avoid --break-system-packages if at all possible, as it can do exactly what it says; it can add packages to the user account which in turn will stop packages on the system from functioning correctly, e.g. random bugs or regressions in users' critical infrastructure as a consequence of using a block in Turtle, (alternatively a virtual environment for the activity),
  • some of your new code is not pep8 compliant, and is therefore slow for us to read, please use flake8,

Hope that helps.

@quozl
Copy link
Contributor

quozl commented Apr 27, 2024

(Also, although Raspberry Pi OS includes distribution packages for use of snap and flatpak, it doesn't have packages for Turtle Art, in turn because Debian does not. I was making derivative debian packages between 2016 and 2017, with a package name of turtleblocks. It was dropped in April 2020 because the work needed to fix it exceeded the time available before a release.)

@44yu5h
Copy link
Author

44yu5h commented Apr 28, 2024

@quozl Thanks for reviewing ^^

some of your new code is not pep8 compliant, and is therefore slow for us to read, please use flake8

I didn't know about this. Thanks, I'll format it

add a clear list of dependencies to README.md, for use by the developers who do packaging

I added the dependencies in readme. However, the dependencies list grows with every addition of sensors, so isn't the req.txt file enough?

avoid --break-system-packages if at all possible, as it can do exactly what it says;
(alternatively a virtual environment for the activity)

I believe it's better to install it globally since other activities too, in the future, will be using the same dependencies.
Also, I read somewhere that if the packages are not already installed it should be fine?
Can you tell me if doing this is the same thing? sudo rm /usr/lib/python3.11/EXTERNALLY-MANAGED

probe for what is needed before making a change; both with raspi-config and with imports, so that if a flatpak, snap, or debian packaging of the software satisfies the requirements your code ends up doing nothing,

I don't know what probing is. If I understand you correctly, it is a test tool that checks for missing dependencies and installs them? Won't that install dependencies for non-Rpi devices too?

Again, thanks for taking out time to review

@44yu5h
Copy link
Author

44yu5h commented Apr 28, 2024

I updated the Readme (relative image links and dependencies) and formatted everything according to pep8.
master...44yu5h:turtleart-activity:master

Let's continue here #94

@44yu5h 44yu5h closed this Apr 28, 2024
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