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

[fernandez85]

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]

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]

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]

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]

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]

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

I agree about exception.

[bialix]

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]

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 ;)

[bialix]

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]

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.

[bialix]

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

[fernandez85]

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.

[bialix]

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]

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

[bialix]

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

[fernandez85]

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

[fernandez85]

@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]

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