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

New Extended Address record type mode saving in 'write_hex_file' function #41

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

fernandez85
Copy link
Contributor

@fernandez85 fernandez85 commented Jun 6, 2020

Hello,
I've finally pushed my changes, which will solve the problem described by me here: #40
There is new parameter 'ext_addr_mode' for 'write_hex_file', which will handle the issue in following way.

  1. ext_addr_mode='linear' (default) - this is old behavior, it will force to use Extended Linear Address records, so it's backward compatible
  2. ext_addr_mode='segment' - this option forces to use Extended Segment Address records, will throw exception, when data is to big to fit 1MB address space or Starting Linear Address is defined
  3. ext_addr_mode='auto' - this option will automatically detect which mode to use, basing firstly on Starting Address record type, secondarily (when there is no Starting Address record) on final data length, will throw exception like in point 2 (whenever 'segment' mode is negotiated).

I did the implementation in few days, but I had problem to manage UnitTest for this change. I hope everything is OK there.

@fernandez85
Copy link
Contributor Author

fernandez85 commented Jun 7, 2020

Hmm, I didn't see that before, but there is a pull-up request (old one though), which is strongly related with this topic: #5
That makes a little different approach. It's more global.
Especially this comment seems to define the implementation:

So, my plan would be like this:

  1. Old name "hex" still be used for automatic decision about using record types if start address present.
  2. Your patch introduces good feature for generating desired format, let's keep it.
  3. Add support for i8hex, i16hex, i32hex into library, as well as to bin2hex convertor.
  4. More tests :-)

Originally posted by @bialix in https://github.com/python-intelhex/intelhe/pull/5#issuecomment-213300462

Right now I'm confused :)
Current implementation (without my changes) supports so called 'i8hex' and 'i32hex' (I don't like the naming, especially they are not official). It's handled automatically. If address spacing is bigger than 16-bit it's 'i32hex' if not it's 'i8hex'.
My change is little more "aggressive". You can force 'i16hex' or 'i32hex', but affect only Extended Address records, leaves Starting Address record untouched (this is current behavior though) . Still if address spacing is below 64k 'i8hex' will be written. So at the end everything is little mixed up.
Finally, I think 'auto' mode which I presented would handle most of the cases. It's only matter to know in which direction this peace of IntelHex should go.
I don't want make any rough moves here. So I will wait for some feedback :)

@bialix
Copy link
Member

bialix commented Jun 9, 2020

OK, my bad. It was 4 years ago. A long time ago in far far galaxy...
IntelHex project was on hold all those years.
Re-reading my comments on that PR I see it was something good about that patch.
Can we take good parts from two?
I need to re-evaluate both patches.

@fernandez85
Copy link
Contributor Author

I see no problem in "merging" both solutions. The only thing here is what You consider as common part and what is need to be changed. This can be done in several variants.
How I see this?

  1. I would stay with new options naming: "linear", "segment" and "auto". For me it's confusing to have options like 'i8hex', 'i16hex', 'i32hex' - only last one tells "truth" about address spacing, 'i8hex' has 16-bit address spacing, 'i16hex' - has 20-bit address spacing. But if You insist to use that notation then no problem at all.
  2. I would only change the name of the new option "ext_addr_mode" => "addr_format" (or similar?)
  3. propagate that parameter into "to_file" method and "bin2hex" as well
  4. Which option will do what (I mark [?], where I'm not sure):
  • no option/default or "auto":
    • if no Starting Address and last address is <64KB hex is saved with no Extended Record
    • if no Starting Address and last address is >=64KB Extended Segment Address records will be used
    • if no Starting Address and last address is >=1MB Extended Linear Address records is used
    • if Starting Segment Address is present and last address is >=64KB then Extended Segment Address records will be used ; if last address is <64KB, then no Extended Address record is needed
    • [?] if Starting Segment Address is present and last address is >=1M then Extended Linear Address records will be used and Starting Address will be converted into Linear (not a real life scenario, but since it's 'auto' format there shouldn't be any errors)
    • if Starting Linear Address is detected and last address is >=64KB then Extended Linear Address record is used; if last address is <64KB, then no Extended Address record is needed
  • "segment" option:
    • [?] if no Starting Address and last address is <64KB hex is saved with no Extended Record (technically no problem to add an Extended Record, but in my opinion it should be kept as simple as it can be)
    • exception should be thrown whenever last address or Starting Address record is >=1MB
    • Starting Linear Address should be converted into Segment if it is <1MB
    • in other cases Starting Segment Address is "passed" and Extended Segment Address records are used (only when last address >64KB)
  • "linear" option:
    • [?] if no Starting Address and last address is <64KB hex is saved with no Extended Record (technically no problem to add an Extended Record, but in my opinion it should be kept as simple as it can be)
    • Starting Segment Address should be converted into linear (even address is <1MB)
    • in other cases Starting Linear Address is "passed" and Extended Linear Address records are used (only when last address >64KB)

And I think that is all. Tell me what You think and I'll start to work on this :)

@bialix
Copy link
Member

bialix commented Jun 10, 2020

OK. Let's do what is minimal accepted for you. I don't want to ask you for something you don't sign in.
Let's keep it simple, right?

Converting start address is overkill for me. It's nice to have, but maybe as a separate method, if any?

Your and older patches are both trying to fix the same problem. Let's name it in scrum terms ))

"I want to have control over the type of extended address record when writing the hex file."
That's all.

What we need, the core part:

  • new option to write_hex_file method. I think something like "extended_addr_type" would be maximal straightforward, based on ihex spec, what do you think? Allowed values for this option would be: linear, segment, auto. You might consider them case insensitive, but this is not strictly required.
  • several unit test to check new behaviour, maybe you need to adapt some older unit test cases to cover new option. Ensure we don't break the old behaviour.
  • in the light of backward compatibility I'd suggest that "linear" type will be default.
  • I'd like to ask you (even urge you) to describe new option in the documentation, see https://python-intelhex.readthedocs.io/en/latest/part2-5.html - my English skill is far from ideal, so any help is highly appreciated.

The second, bonus part:

  • propagate new option to bin2hex utility
  • describe it in help and manual ))

So, about "auto" type of extended address record. It looks for me that we must check the maximum address of the data first, then if still segment is possible - check starting address record type, if no starting address record here - use linear. As we say before - linear was default behaviour in all previous releases.

Does it make sense for you?
If I don't answer some of your questions - please, ask again.

I'd like to summarize, for every change (core and bonus parts) I'd like to have from your patch:

  • the code itself
  • unit tests (where applicable)
  • documentation please, very please

@bialix
Copy link
Member

bialix commented Jun 10, 2020

"segment" option:
exception should be thrown whenever last address or Starting Address record is >=1MB

I agree about exception.

@bialix
Copy link
Member

bialix commented Jun 10, 2020

Umm, and another option for auto. I think we can save the extended address type we read from source hex file. If there is only one type, you can save it for later use as another sign before inspecting starting addr type. This would make round trip better. Again - it's up to you.

@fernandez85
Copy link
Contributor Author

fernandez85 commented Jun 10, 2020

Hi,
thanks for feedback :)
I have a question. Have You check the code in this pull-up request? Most of that You wrote is done (core part). Only few details to change, like parameter name, etc. But You could check it is this what You meant.
Now about your propositions/suggestions:

So, about "auto" type of extended address record. It looks for me that we must check the maximum address of the data first,

this is already done in my solution, but:

then if still segment is possible - check starting address record type, if no starting address record here - use linear. As we say before - linear was default behaviour in all previous releases.

I can agree to preserve old the behavior, when possible. Nevertheless 'auto' mode is something new, which will be used only by people who will no something about it. Default option is 'linear', just like did with my current change.
Here is a dilemma, what is more important? Size of data or starting address type. In 'auto' mode I would prefer something like this:

  • if data <64KB (no matter if there is any Starting Address) there would be no Extended Address records
  • if data >=64KB and <1MB and Starting Address is Segment type then output Extended Address records are also Segment type
  • if data >=1MB and Starting Address is Segment type then output Extended Address records are also Linear type; leave Starting Address untouched (old behavior)
  • if data >=64KB and Starting Address is Linear type then output Extended Address records are also Linear type (this is also back-compatible)

'linear' option would be actually what we have in IntelHex right now. 'segment' would throw exception whenever Starting Linear Address is detected or data >=1MB. Both would not add any Extended Address records when when data <64KB.
(Unless we consider Starting Address conversion (only for output), but this will break a little bit current implementation assumption. And may not be so easy as just simple data format conversion)
Currently my changes doesn't include all this. I think this is a good compromise.

Umm, and another option for auto. I think we can save the extended address type we read from source hex file. If there is only one type, you can save it for later use as another sign before inspecting starting addr type. This would make round trip better. Again - it's up to you.

I was thinking about this. But first of all the IntelHex internal structure data need to support this. And we need to always remember about this if we want to rely on that parameter.
I prefer what we currently have - we deal with Extended Address records when we want to export data int HEX file, if needed (>=64KB data only).

I would like add additionally, that besides 'bin2hex' we need to take care of 'hexmerge' as well - it uses 'write_to_hex' directly. I checked it today ;)

intelhex/__init__.py Outdated Show resolved Hide resolved
intelhex/__init__.py Outdated Show resolved Hide resolved
@@ -630,26 +659,48 @@ def write_hex_file(self, f, write_start_addr=True, eolstyle='native', byte_count
minaddr = addresses[0]
maxaddr = addresses[-1]

if extaddr_rectyp == -1:
Copy link
Member

Choose a reason for hiding this comment

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

I think we should check maxaddr before checking start addr record type.

Copy link
Contributor Author

@fernandez85 fernandez85 Jun 15, 2020

Choose a reason for hiding this comment

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

Reason why this is here, is because whole data are manged here. We have all necessary variables here. If we want to make checks for 'maxaddr' before Starting Address then I'm understanding this that You want to make this more important criteria than Start Address type for Extended Address resolving. If this was Your intention, then no problem - I will do a little change and refactor. If not then I suggest this would stay as it is now.

cur_addr = minaddr
cur_ix = 0

while cur_addr <= maxaddr:
if need_offset_record:
if cur_addr > 0x0FFFFF and extaddr_rectyp == 2:
Copy link
Member

Choose a reason for hiding this comment

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

we should check this condition at the start of the operation. checking it here every time is waste of time.
if maxaddr > 0x0FFFFF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, agree. Need to find different suitable exception type :)

bin[4] = b[0] # msb of high_ofs
bin[5] = b[1] # lsb of high_ofs
bin[5] = b[1] # lsb of high_ofs
Copy link
Member

Choose a reason for hiding this comment

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

don't add extra white spaces please

@bialix
Copy link
Member

bialix commented Jun 15, 2020

I have a question. Have You check the code in this pull-up request?

Sorry, I haven't. Done now, for core part, except tests.

@fernandez85 fernandez85 marked this pull request as draft June 18, 2020 18:59
@fernandez85
Copy link
Contributor Author

I've added a new option for 'ext_addr_mode' - 'none'. When selected it only checks if last address fits into 64KB. It's just to have consistent solution for all options ('segment', 'linear').
If You feel that naming for that option is not good, please let me know.

@fernandez85 fernandez85 marked this pull request as ready for review June 26, 2020 07:50
@fernandez85 fernandez85 requested a review from bialix July 8, 2020 22:19
Copy link
Member

@bialix bialix left a comment

Choose a reason for hiding this comment

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

Please add something to NEWS and AUTHORS

BLOCK_SIZE = 128 # 128 bytes
for addr in range(0, EEPROM_SIZE, BLOCK_SIZE):
eeprom.i2c_write(addr, ih.tobinarray(start=addr, size=BLOCK_SIZE))
Writing out data
Copy link
Member

Choose a reason for hiding this comment

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

I spot different line-endings here. Can you double check please? I'd like to see actual changes in document, but diff shows me that all document was changed. Not good. I believe all my files are using LF line-endings. You can set git attributes for those text files if you wish. But please, don't blindly change line endings. It's bad practice.

Copy link
Contributor Author

@fernandez85 fernandez85 Jul 9, 2020

Choose a reason for hiding this comment

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

OK, I will recheck this. I checked few files and I thought they are OK.
Maybe the problem is that I'm using Windows and it does some "magically" things with the files :/

Copy link
Member

Choose a reason for hiding this comment

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

I'm on Windows too. You can use smart text editor though, Visual Studio Code is very good and should preserve original line-endings IIRC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use VS Code as well. It looks like something went wrong.


eol = IntelHex._get_eol_textfile(eolstyle, sys.platform)

addresses = dict_keys(self._buf)
Copy link
Member

Choose a reason for hiding this comment

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

Looks good.

@bialix
Copy link
Member

bialix commented Jul 9, 2020

I can't see unit tests for new feature. Can you add some?

@fernandez85
Copy link
Contributor Author

fernandez85 commented Jul 9, 2020

I can't see unit tests for new feature. Can you add some?

Test for new 'none' option are there. I didn't do special test suite for it. Just added/changed to which I created before.
If You do simple text search over 'test.py' file using this keyword: 'none', then You will find tests for that feature.

intelhex/test.py Outdated Show resolved Hide resolved
@bialix
Copy link
Member

bialix commented Jul 9, 2020

I haven't noticed changes to tests - because it says diff too large. Again line edings. Please fix it.
I would recommend to make a fresh start, or squash commits together after fixing line endings.

@fernandez85
Copy link
Contributor Author

You mean to squash all the commits in this pull request? Is this even possible after everything is pushed on repo?

@bialix
Copy link
Member

bialix commented Jul 9, 2020

I think you will need to use git push --force or something like that.

New 'ext_addr_mode' parameter responsible for Extended Address records.
@fernandez85
Copy link
Contributor Author

I left 'X' in 'NEWS' on purpose, as I can't predict the future ;)

@fernandez85
Copy link
Contributor Author

fernandez85 commented Oct 21, 2020

@bialix I don't know if You get the notifications I had corrected everything that you requested. It's waiting for You to review :)

@bialix
Copy link
Member

bialix commented Oct 22, 2020

Sorry for delay! I can't review it right now. I'll try my best soon.

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.

does it support extended segment address write?
2 participants