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

A1 cam updates #178

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

A1 cam updates #178

wants to merge 6 commits into from

Conversation

Jonathan-Woo
Copy link
Contributor

@Jonathan-Woo Jonathan-Woo commented Feb 28, 2025

Generally cleaned stuff up. Added logging, autofocus, quality options.

@Jonathan-Woo Jonathan-Woo requested a review from sgbaird February 28, 2025 23:58
@Jonathan-Woo
Copy link
Contributor Author

Jonathan-Woo commented Mar 1, 2025 via email

Copy link
Member

@sgbaird sgbaird left a comment

Choose a reason for hiding this comment

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

The logic seems sound - overall was easy to follow the script. Few minor points and clarifications listed. Overall, it seems like a lot of logging - I'm guessing because it's operating in a headless state, usually unsupervised, and you want to keep track of it for when you go into the device to check?

I've certainly been one to implement this style at times, but usually I start simple, try to do some stress testing, and then add try-except logic as needed (rather than anticipating). For example, I iterated a lot on https://github.com/sparks-baird/self-driving-lab-demo/blob/main/src/public_mqtt_sdl_demo/main.py because it was pretty fragile and hard to debug without the logging. Some key commits: https://github.com/sparks-baird/self-driving-lab-demo/blob/6dc95458027962a303e93390992c34f7f1b9c262/src/public_mqtt_sdl_demo/main.py --> sparks-baird/self-driving-lab-demo@8103b54

I don't know as well as you what all went wrong or is likely to go wrong (and what is making it easier to maintain) and I know you've done a lot of troubleshooting and debugging with the cameras, so I'll defer to you on this one. Just trying to get a handle on these topics within the training lab.

picam2.capture_file(file_path)
logging.info("Successfully capture image")
except Exception as e:
logging.error(f"Error capturing image: {e}")
Copy link
Member

Choose a reason for hiding this comment

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

Did you run into errors here at any point?

Copy link
Member

Choose a reason for hiding this comment

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

Is the public access setup such that anyone can read files if given the URI, but access is restricted for the upload of files? (i.e., requires authentication)

picam2.autofocus_cycle()
logging.info("Successfully finished autofocus")
except Exception as e:
logging.error(f"Error autofocusing: {e}")
Copy link
Member

Choose a reason for hiding this comment

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

Did you run into errors here at any point?

except Exception as e:
logging.error(f"Error capturing image: {e}")

# make sure to setup S3 bucket with ACL and public access so that the link works publically
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for comments like these!

)
logging.info("Successfully uploaded to S3")
except Exception as e:
logging.error(f"Error uploading to S3: {e}")
Copy link
Member

Choose a reason for hiding this comment

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

Did you run into errors here at any point?

client.publish(CAMERA_WRITE_ENDPOINT, json.dumps({"image_url": file_uri}))
logging.info(f"Published {file_uri} to {CAMERA_WRITE_ENDPOINT}")
except Exception as e:
logging.error(
Copy link
Member

Choose a reason for hiding this comment

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

Did you run into errors here at any point?

picam2.set_controls({"AfMode": "auto"})
# JPEG quality level (0 is worst, 95 is best)
picam2.options["quality"] = 90
config = picam2.create_still_configuration(transform=Transform(vflip=1))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe comment that the vflip is because we use a camera mount that puts the camera upside down

object_name = datetime.utcnow().strftime("%Y-%m-%d-%H:%M:%S") + ".jpeg"
logging.info("Begin upload to S3")
try:
s3.upload_file(
Copy link
Member

Choose a reason for hiding this comment

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

How is s3 being used prior to being defined? If it were a global variable, I'd understand, but I only see it being defined in the if __main__ towards the end. Maybe I'm missing something basic.

logger = logging.getLogger("device")


def on_message(client, userdata, message):
Copy link
Member

Choose a reason for hiding this comment

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

Any thoughts about queuing in this context? I.e., what will happen if it gets a request while it's still processing another (will it be ignored completely, will it sit in a queue, etc.?). This is more of a fringe case atm, but I wanted to double check what the expected behavior is

I think I designed https://ac-microcourses.readthedocs.io/en/latest/courses/hello-world/1.4-hardware-software-communication.html and the corresponding assignment microcontroller script to handle requests in a FIFO style, but the use-case is somewhat different and that uses MicroPython mqtt_as instead of Python paho-mqtt (see also orchestrator nb).

import logging
import sys

logging.basicConfig(
Copy link
Member

Choose a reason for hiding this comment

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

Could you comment on the reasoning behind the more involved logging setup? (file handler, stream handler, why default basicConfig doesn't suffice, etc.). I imagine something to do with the Pi using a headless OS and having a file in case of unexpected shut-down, while still being able to see the stdout when you're SSH'd into it and debugging/prototyping.

from libcamera import controls, Transform

from my_secrets import (
CAMERA_READ_ENDPOINT,
Copy link
Member

Choose a reason for hiding this comment

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

In this case because it's MQTT, let's call this a topic rather than an endpoint

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.

2 participants