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

Disabling code or data flash. Intel HEX support. #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bicyclesonthemoon
Copy link
Contributor

I added the possibility to disable data flash or code flash operations as commented on issue #9

I added two commandline options. -x and -y.
And two variables char nodata and char nocode.
Using -x will disable any data flash operations.
Using -y will disable any code flash operations.

Also I added a support for Intel HEX format.
Previously I had to use srec_cat to convert hex files to s-record before using fl78flash.
So I made it possible to use the files directly.
A new comand line option is -f.
-f 0 is S-record.
-f 1 is Intel HEX
default is 0 (S-record).
I added ihex_read() function which is a copy or srec_read() slightly modified for the Intel HEX format.

@msalau
Copy link
Owner

msalau commented Jan 11, 2019

Hi @bicyclesonthemoon

Thanks for your effort!
I have few notes about the Intel hex file type support:

  1. The function should be in a separate file (say ihex.c), since it is not obvious that ihex_read() is located in srec.c
  2. ihex_read() should use it's own error code definitions, again to not to confuse readers and the list for hex file type may differ.
  3. File type format should be deducted from its extension, not by a command line argument.
    Possible extensions for srec: .s19, .s28, .s37, .srec, .mot, .mxt (from wiki but dropped few that may be confusing, e.g. .s is used for assembly files too);
    For hex: .ihex, .hex

Also you may split the pull request into 2:

  1. partial flashing code/data
  2. Intel hex file type support

I'm perfectly fine with implementation of the first one and will merge it immediately.

Regards,
Maksim.

@bicyclesonthemoon
Copy link
Contributor Author

bicyclesonthemoon commented Jan 14, 2019

I have created a separate pull request for data/code flash disabling.

As for the ihex support, I didn't create additional locations or definitions because I didn't see the need for it because of how almost identical these two formats are.
But of course it can be done.

I do not like the idea of deciding the file format based on filename extension.
The filename is a separate thing from the file and there is no guarantee that the file will have a matching extension.

If we want automatic detection of file type this can be done based of the file content.
This is simple:
S-record has lines starting with "S" followed by hexadecimal numbers,
Intel HEX has lines starting with ":" followed by hexadecimal numbers.

@msalau
Copy link
Owner

msalau commented Jan 16, 2019

Hi @bicyclesonthemoon

I have created a separate pull request for data/code flash disabling.

Thanks! I've merged it already.

As for the ihex support, I didn't create additional locations or definitions because I didn't see the need for it because of how almost identical these two formats are.
But of course it can be done.

Indeed, S-Record and Intel Hex file formats have much in common, but they are different entities and thus should be separated. It is counter-intuitive to place the hex file handler in a file named srec.c

I do not like the idea of deciding the file format based on filename extension.
The filename is a separate thing from the file and there is no guarantee that the file will have a matching extension.

I believe extensions were invented to define file types :) Thus I insist on this feature. I also understand your motivation for adding an option to define the file type in command line. Some tools do use awkward extensions instead of commonly used ones.
So lets do the following: add an option to override automatic extension-based file type detection. And if the override option is not present - use extension to determine file type.
E.g.: Intel hex file reader will be used in the following cases

rl78flash ... -f ihex ./foobar.baz
rl78flash ... -f ihex ./foobar.srec
rl78flash ... ./foobar.ihex

I think textual file types are easier for end-users.

Regards,
Maksim.

@bicyclesonthemoon
Copy link
Contributor Author

Hello again,
I would like to return to this topic and introduce the understanding of intel hex fianlly,
but this time I would like to introduce it in such way that will actually be accepted by you.

So I would like to ask, what is your vision of how it should be.

  1. ihex_read() should not be in srec.c
    Ok, so either:
    a) make ihex.c and ihex.h and place the function there.
    (but then where is the rignt place for ascii2hex()? both srec and ihex functions need it)
    b) keep in the same file but change the name to something not srec-specific?

  2. Determining file format.
    Yes, I agree text-names for file formats make more sense,
    so -f srec, -f ihex. ok.
    Actually I used numbers because that's what you did with the mode -m 0, -m 1, -m2, -m3.
    (actually in my opinion it makes more sense to thave two separate parameters for 1/2 wire and dtr/rts reset, especially that reset invert is a saeparate parameters, but at this point -m is already estabilished so change would break compatibility)

How to detect file format?
I see 3 possibilities:
a):

  • use -f srec, -f ihex to determine format, error if -f somethingelse.
  • if no -f , assume srec.
    b):
  • use -f srec, -f ihex to determine format, error if -f somethingelse.
  • if no -f use file extension:
  • if ".hex", ".ihex" extension assume ihex
  • anything else assume srec
    c):
  • use -f srec, -f ihex to determine format, error if -f somethingelse.
  • if no -f check first line of file:
  • if starts with ":" assume ihex,
  • if starts with "S" assume srec

Which way to go?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants