search

zrafa / xinu-avr

the Xinu OS for AVR atmega328p devices (e.g. Arduino)
http://se.fi.uncoma.edu.ar/xinu-avr
Other
32 stars 7 forks source link

Add error messages #2

Closed Darin755 closed 4 years ago

Darin755 commented 4 years ago

I uncommitted the print statements for all of the error messages

zrafa commented 4 years ago

Hi, thanks for the pull request. I have not read all the diffs yet. But some comments: The most of const chars for printfs are using a lot of flash, and we have few bytes left. The project still needs a block device driver for eeprom, a tiny filesystem on that device, and more features to the editor and basic to write files using the filesystem. All that code will use flash, and there is almost no room. What do you think? Is it too important for you the error messages for the shell?

Darin755 commented 4 years ago

I have given it some thought. I think it would be best to leave all of the error messages out except for the command not found in the main shell. I will update the pull request accordingly.

Darin755 commented 4 years ago

I updated it to only have command not found error. Please review.

zrafa commented 4 years ago

I have several comments.

I do not know how to download the raw/txt pull request (I use github for remote server backup and publish, pero dont like so much the web interface). Do you know how to download the plain txt of the commits from this pull request? Maybe there are some git commands, but dont know them neither.

Now, several questions:

I do not understand well the WEB content of the pull request, but

Is it showing that you are adding temporary files? like main/main.c~ ? there are others files ending with ~, so I guess that there are temp files. If your commits are adding them, then the commit is wrong.

Is it showing that you are adding .o files? I myself do that error from time to time, but anyway, that is wrong, and the commit must not add .o files or binary files.

Is it showing that you are modifying conf/ files? The commit says that it uncomments printf, but then the commit shows diffs from conf/ files. If your commits are modifying them, then the commit is wrong.

Finally, take in count that the main shell code lives under apps/. Under main/ the final user will put any app from apps/ folder. So if you want permanent changes for the shell app then you should change the files (sync) under apps/shell as well. In fact, in some future, I will delete the current shell under main/. But I am waiting for a while, because some users will like to download the code and build like it is.

Darin755 commented 4 years ago

I seems that the web interface does not update the pull request when I update the fork from the command line. I will open a new pull request.