search

JasonSchoon / stm32flash

Automatically exported from code.google.com/p/stm32flash
0 stars 0 forks source link

Does not support reading/writing stdin/stdout #22

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
The stm32flash tool does not currently support reading to/from the stdout/stdin 
streams.

To use stdin/stdout, specify '-' as the filename. For device writes, the size 
of the input stream must be specified.

See attached patch that adds support for this behavior.

Original issue reported on code.google.com by reubendowle0@gmail.com on 5 Jan 2012 at 8:09

Attachments:

GoogleCodeExporter commented 9 years ago 
Here is a patch that is actually against the latest cvs code

Original comment by reubendowle0@gmail.com on 5 Jan 2012 at 9:54

Attachments:

GoogleCodeExporter commented 9 years ago 
This is great, but instead of suppressing the output, make it print to stderr

Original comment by ge...@spacevs.com on 13 Jan 2012 at 10:47

GoogleCodeExporter commented 9 years ago 
In absence of the maintainer, I have set up a git repository where I will try 
to incorporate all outstanding patches: 
https://gitorious.org/stm32flash/stm32flash#more

I have started on this issue, in the "issue22" branch, by making it print to 
stderr.

However I am not happy with the compulsory -size option. Is is not possible to 
just read in from stdin until EOF or end of flash? If you can submit a new 
patch, prepared using git format-patch, and on top of the issue22 branch, it 
would be great.

Original comment by lists.to...@gmail.com on 17 Nov 2012 at 8:49

GoogleCodeExporter commented 9 years ago 
I think you are right - eliminating the size option would be a better solution, 
and relatively easy to implement.

I don't think writing to standard error is a good idea though. There are use 
cases where you may wish to parse the output of the program in a script, and 
this is complicated (slightly) by putting that information to standard err. We 
should stick to the normal unix behaviour of messages to stdout, errors to 
stderr.

Personally, I think the following would be best:
 - For most situations, print to std out as normal
 - If reading to stdout, then put messages to stderr
 - Still have a quiet option, but don't automatically turn it on for stdout reads (just switch to using stderr for the messages).

Original comment by reubendowle0@gmail.com on 18 Nov 2012 at 6:56

GoogleCodeExporter commented 9 years ago 
OK, feel free to ignore that "stderr" commit of mine. Can you give an example 
of a program that behaves like your suggestion? I think there are programs 
where you can ask for diagnostics and progress to be written to a fourth file 
descriptor. But I don't know if this will be portable - can Windows do this?

Original comment by lists.to...@gmail.com on 18 Nov 2012 at 8:41

GoogleCodeExporter commented 9 years ago 
I can't think of an example off the top of my head. However I think that while 
useful for my specific purposes, the reading to stdout is not likely to be 
something may people use. So I think that making the behaviour of the program 
in the typical case (reading/writing from files) present information on stdout 
would be more like what most people expect. My original patch just turned off 
info when stdout was used for data, but I think that is probably too 
restrictive.

Now I think about it, a common counter example would be dd, which always uses 
stderr to print its information. But with this software, a typical user might 
want for example to parse the output for what type of chip is connected, which 
would need a bunch of redirection if we use stderr.

Original comment by reubendowle0@gmail.com on 18 Nov 2012 at 9:09

GoogleCodeExporter commented 9 years ago 
For now I changed my preparatory patch to preserve the old behaviour, using a 
new stream pointer "diag" for these messages which is set to stdout by default.

Original comment by lists.to...@gmail.com on 19 Nov 2012 at 8:40

GoogleCodeExporter commented 9 years ago 
See attached for a series of patches that incorporates all my local changes to 
stm32flash. They are not all strictly related to this issue, but I thought it 
was easier to just attach them all here.

Original comment by reubendowle0@gmail.com on 19 Nov 2012 at 8:20

Attachments:

GoogleCodeExporter commented 9 years ago 
Thanks for separated, formatted patches! However it will be easier for us if 
track at least 0002 and 0003 in separate issues. I think 0003 overlaps with 
issue 17 - can you please compare with the patch there and merge them if needed?

0001: I thought this would not be needed if we will redirect diag to stderr 
when dumping read data to stdout, like you already implemented in 0004. Any 
other reason for a quiet option?

0004: I will look closer at this later. I just had a quick look at the patch so 
I missed the context, but why do you accept zero length when reading from stdin?
Also, I bet stdin and stdout support are completely independent so it would be 
easier to review if they are two separate patches...

0005: Maybe it would be nicer to use a separate option (-s?) for address:length 
instead of adding it to the filename? These are memory addresses and not file 
offsets, right?

Original comment by lists.to...@gmail.com on 19 Nov 2012 at 9:41

GoogleCodeExporter commented 9 years ago 
I suggested -s for 0005 but that one is already taken. Maybe -S since it is an 
alternative to -s? Or -a for --address. If we really want to be cheap with 
options we could also interpret -s values >= 0x08000000 as addresses.

Thanks for working on this! I found the -s and -e options difficult and 
confusing when I started using stm32flash.

Original comment by lists.to...@gmail.com on 19 Nov 2012 at 9:48

GoogleCodeExporter commented 9 years ago 
I guess I can split this out to different issues. I am a bit busy now, so might 
not get to any more changes for a little while. But on your specific points:

0003: Now I actually remember that this IS the change in issue 17 - I think I 
ported that patch to my local tree. Anyway, if the other person for that issue 
has gone missing, I will just give you a nicely formatted patch on that issue.

0001: This is not strictly required for this. But it is helpful for me - I use 
stm32flash inside a script for checking and updating firmware, and I just want 
that to show checking firmware... OK|FAIL. I could do this with redirect to 
null, but many other tools have a quite option for this purpose, so I thought I 
would add it here as well. Perhaps better to split as a seperate issue. 
Although that will just increase your merging workload I think...

0004: reads will return 0 when no more data is available. If we read to the end 
we need to not enter an infinite loop (since we have now eliminated the size 
parameter we will always have a zero-length read at the end). A zero read is 
only an error if we have already read part of a block for writing.

0005: Sure. I will look at splitting address and length off to separate options.

Original comment by reubendowle0@gmail.com on 20 Nov 2012 at 2:07

GoogleCodeExporter commented 9 years ago 
0001: Yes, please take it to another issue. I am reluctant to applying it since 
it does not bring much value over >/dev/null and it changes a lot of code 
(possibly breaking other outstanding patches). I would like to merge the other 
stuff first anyway. Would attaching diag to /dev/null do the same trick, or do 
you leave some messages in?

0005: I find the address:length syntax good, so one new option is fine with me.

If you split 0004 I will apply them rather fast... I hope to get more work done 
on this in the weekend.

Original comment by lists.to...@gmail.com on 21 Nov 2012 at 9:46

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
Ok, have a look at this series. Just the minimal patches to add stdin/stdout 
support. Split into one for read and one for write.

Original comment by reubendowle0@gmail.com on 23 Nov 2012 at 12:58

Attachments:

GoogleCodeExporter commented 9 years ago 
Excellent! Applied them to my "merging branch" - after my own testing and 
getting name/addresses for some other patches that will go to my master.

BTW, I guess you could attach your new 0005 to issue 23.

Original comment by lists.to...@gmail.com on 24 Nov 2012 at 1:24

GoogleCodeExporter commented 9 years ago 
This has now been fixed in the official repository and is included in version 
0.3beta2.

Original comment by lists.to...@gmail.com on 9 Dec 2013 at 11:12