Skip to content

Conversation

@evadethomas
Copy link

Description:

This pull request contains a guy graphics library that contains animations for the user's enjoyment. As they open and run commands in FogOS, the animations will run. This library gives the user directions on how to run the animations. Future developers can include any of the functions within the interface in any file. To see some examples, look in and run ls.c and rm.c. There is also a message function that is optional, but calling it almost takes more work than writing your own message so I just left it in.

This PR also implements another syscall function called sleep_ms, that allows programs to sleep for shorter durations than the current sleep functions in FogOS. To see more documentation, look in docs, guy.md.

Changed files:

Makefile - now includes guy.c
guy.c - main function that calls on the guy library
guy2.c - the actual guy library, provides an interface so functions can be accessed within user.h
user.h - now contains guy functions
docs/guy.md - for documentation
sysproc.c, syscall.c, syscall.h - all changes to implement ms_sleep

Limitations:
Never actually use the message function but left it in for fun.
Sleep_ms can occasionally interfere with animations if there is a lot running on your machine. Keep this in mind. If this is the case It's not my code that is trash its your CPU or wifi. :P

Occasionally exec fails, but I have also been informed that is not a result of my code and more what the system can handle.

Example:

The Professor let me know that I don't need to make a test script because of the kind of project that I did, so instead here are a few screen shots:

When running make qemu:
exampleReview

When user runs rm :
removing

When user enters guy info:
info

@rick2244
Copy link

I will code review this.

@rick2244
Copy link

rick2244 commented Oct 1, 2024

Guy Graphics Library

guy.c: formatting issues
guy2.c: maybe consider having a global variable that is the guy, so that you can plug that in since you are doing the same thing multiple times across different methods

formatting issue with guy_move_head(), guy_run(), guy_celebrate(),
guy_celebrate_info()

user.h: no issues
docs: helpful info in understanding how your code works and your decisions
sysproc.c: would be helpful to include some useful comments
syscall.c: no issues
syscall.h: no issues

Pretty cool animations when make qemu is booted.

More comments on other methods and Javadoc documentation of what each method does. It would also be helpful to include more information on how to run certain parts of your project. It took a little bit of time to understand how to run it.
For future possible work, consider adding a flag to it so that it can be turned on and off for the processes it is connected to.

High Level Checks:

  • [✔️] PR fully resolves the task/issue
  • [✔️ ] Does the PR have tests? Could have probably created test files and directories for me to work with
  • [] Does the PR follow project code formatting standards?
  • [✔️] Does the OS still compile and run?
  • [✔️] Is documentation included?

Code Checks:

  • [✔️] Does the code achieve its intended purpose?
  • [✔️] Were all edge cases considered?
  • [✔️] Is the code organized and maintainable?
  • [✔️] Does the code maintain the same level of performance? (no performance regressions)

@dtruong33
Copy link

I can code review this

@dtruong33
Copy link

High Level Checks:

  • PR fully resolves the task/issue
    --- Guy is fully implemented and shown
  • Does the PR have tests?
    --- No tests needed
  • Does the PR follow project code formatting standards?
    --- Follows formatting overall
  • Does the OS still compile and run?
    --- OS compiles and runs as normal
  • Is documentation included?
    --- No javadocs

Code Checks:

  • Does the code achieve its intended purpose?
    --- Code displays a variety of animations as intended
  • Were all edge cases considered?
    --- No edge cases found
  • Is the code organized and maintainable?
    --- Well organized and readable
  • Does the code maintain the same level of performance? (no performance regressions)
    --- No performance losses beyond the expected from animation

As a whole this is a fun and well implemented addition to XV6. It is well organized to add onto it as well as being easily used by other functions to add their own animations of guy. I also found no edge cases or bugs with the code with my testing. However, documentation could be improved with javadocs with every function to better explain their purposes and how they fit with one another. Additionally, formatting can be improved such as with the one line printf in guy_info() that can be broken up. But overall this is a solid project that can be added and added upon to in any os of XV6.

@eshadupz
Copy link

eshadupz commented Oct 5, 2024

I can code review this!

@eshadupz
Copy link

eshadupz commented Oct 5, 2024

High Level Checks:

  • [PR fully resolves the task/issue
  • Does the PR have tests?
  • Does the PR follow project code formatting standards?
  • Does the OS still compile and run?
  • Is documentation included?

Code Checks:

  • Does the code achieve its intended purpose?
  • Were all edge cases considered?
  • Is the code organized and maintainable?
  • Does the code maintain the same level of performance? (no performance regressions)

I love this project! It's very fun to see guy pop up when qemu is booted- I think in terms of high level performance, it's great but when I mistype a command, I think the error message should include some valid inputs to type in so that the user can follow that- so for example: "mistyped - try 'guy run', 'guy ___'," etc.... instead of just "mistyped try again".

Code quality looks good! Maybe instead of a clear screen function since that wipes away everything, maybe create a separate function that just clears away the animation instead of the instructions to run the program. Your documentation is easy to follow, the instructions are great. Awesome job!

@malensek
Copy link
Contributor

malensek commented Oct 22, 2024

This is definitely one of the more unique additions to user space! The other CRs already mentioned it, but this needs a formatting sweep to get in xv6 formatting and fix some of the spacing issues (tabs/spaces mixed).

Is there any reason the system call is needed? I think that can be implemented by calling the regular sleep from user space.

I know this is intended to be used as a library or user space program, but the guy / guy2 naming is a bit confusing. Maybe guy and guy_lib?

I like Esha's idea to print the guy and then move the cursor back so the screen doesn't need to be cleared (selectively clearing it instead) but I imagine that would be a lot of work and isn't a required update.

you can get +0.5 back for fixing the formatting and issues outlined above.

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.

5 participants