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

fernandez85 · 2020-06-06T11:22:36Z

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.

ext_addr_mode='linear' (default) - this is old behavior, it will force to use Extended Linear Address records, so it's backward compatible
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
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 · 2020-06-07T00:12:04Z

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:

Old name "hex" still be used for automatic decision about using record types if start address present.

Your patch introduces good feature for generating desired format, let's keep it.

Add support for i8hex, i16hex, i32hex into library, as well as to bin2hex convertor.

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 · 2020-06-09T11:29:10Z

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 · 2020-06-09T21:09:09Z

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?

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.
I would only change the name of the new option "ext_addr_mode" => "addr_format" (or similar?)
propagate that parameter into "to_file" method and "bin2hex" as well
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 · 2020-06-10T12:08:38Z

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 · 2020-06-10T12:18:53Z

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

I agree about exception.

bialix · 2020-06-10T12:22:53Z

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 · 2020-06-10T15:34:48Z

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

bialix · 2020-06-15T16:33:10Z

intelhex/__init__.py

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


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

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.

bialix · 2020-06-15T16:34:52Z

intelhex/__init__.py

            cur_addr = minaddr
            cur_ix = 0

            while cur_addr <= maxaddr:
                if need_offset_record:
+                    if cur_addr > 0x0FFFFF and extaddr_rectyp == 2:


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

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

bialix · 2020-06-15T16:35:16Z

intelhex/__init__.py

                    bin[4] = b[0]   # msb of high_ofs
-                    bin[5] = b[1]   # lsb of high_ofs
+                    bin[5] = b[1]   # lsb of high_ofs                    


don't add extra white spaces please

bialix · 2020-06-15T16:37:09Z

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 · 2020-06-22T21:38:30Z

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

Please add something to NEWS and AUTHORS

bialix · 2020-07-09T07:06:18Z

docs/manual/part2-5.txt

-      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


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.

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 :/

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.

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

bialix · 2020-07-09T07:24:21Z

intelhex/__init__.py


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

+        addresses = dict_keys(self._buf)


Looks good.

bialix · 2020-07-09T07:59:34Z

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

fernandez85 · 2020-07-09T10:54:30Z

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

bialix · 2020-07-09T10:58:36Z

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 · 2020-07-09T11:30:41Z

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

bialix · 2020-07-09T11:33:47Z

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 · 2020-07-10T22:42:00Z

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

fernandez85 · 2020-10-21T16:47:47Z

@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 · 2020-10-22T14:56:36Z

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

bialix requested changes Jun 15, 2020

View reviewed changes

fernandez85 marked this pull request as draft June 18, 2020 18:59

fernandez85 marked this pull request as ready for review June 26, 2020 07:50

fernandez85 requested a review from bialix July 8, 2020 22:19

bialix reviewed Jul 9, 2020

View reviewed changes

bialix mentioned this pull request Jul 9, 2020

does it support extended segment address write? #28

Open

bialix linked an issue Jul 9, 2020 that may be closed by this pull request

does it support extended segment address write? #28

Open

bialix mentioned this pull request Jul 9, 2020

Added support for segment addresing in output hex files #5

Open

bialix requested changes Jul 9, 2020

View reviewed changes

intelhex/test.py Outdated Show resolved Hide resolved

fernandez85 added 2 commits July 11, 2020 00:38

Add Extended Address resolver parameter to 'write_hex_file' function

d43e1ed

New 'ext_addr_mode' parameter responsible for Extended Address records.

Update AUTHORS and NEWS

1d8f5c8

fernandez85 force-pushed the writeToHex_extAddrOpt branch from 88a2e42 to 1d8f5c8 Compare July 10, 2020 22:40

fernandez85 requested a review from bialix July 10, 2020 22:41

Merge branch 'master' into writeToHex_extAddrOpt

485b60a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fernandez85 commented Jun 6, 2020 •

edited

Loading

fernandez85 commented Jun 7, 2020 •

edited

Loading

bialix commented Jun 9, 2020

fernandez85 commented Jun 9, 2020

bialix commented Jun 10, 2020 •

edited

Loading

bialix commented Jun 10, 2020

bialix commented Jun 10, 2020 •

edited

Loading

fernandez85 commented Jun 10, 2020 •

edited

Loading

bialix Jun 15, 2020

fernandez85 Jun 15, 2020 •

edited

Loading

bialix Jun 15, 2020

fernandez85 Jun 15, 2020

bialix Jun 15, 2020

bialix commented Jun 15, 2020

fernandez85 commented Jun 22, 2020

bialix left a comment

bialix Jul 9, 2020

fernandez85 Jul 9, 2020 •

edited

Loading

bialix Jul 9, 2020

fernandez85 Jul 9, 2020

bialix Jul 9, 2020

bialix commented Jul 9, 2020

fernandez85 commented Jul 9, 2020 •

edited

Loading

bialix commented Jul 9, 2020 •

edited

Loading

fernandez85 commented Jul 9, 2020

bialix commented Jul 9, 2020

fernandez85 commented Jul 10, 2020

fernandez85 commented Oct 21, 2020 •

edited

Loading

bialix commented Oct 22, 2020


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

		addresses = dict_keys(self._buf)

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

Are you sure you want to change the base?

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

Conversation

fernandez85 commented Jun 6, 2020 • edited Loading

fernandez85 commented Jun 7, 2020 • edited Loading

bialix commented Jun 9, 2020

fernandez85 commented Jun 9, 2020

bialix commented Jun 10, 2020 • edited Loading

bialix commented Jun 10, 2020

bialix commented Jun 10, 2020 • edited Loading

fernandez85 commented Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

fernandez85 Jun 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bialix commented Jun 15, 2020

fernandez85 commented Jun 22, 2020

bialix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fernandez85 Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bialix commented Jul 9, 2020

fernandez85 commented Jul 9, 2020 • edited Loading

bialix commented Jul 9, 2020 • edited Loading

fernandez85 commented Jul 9, 2020

bialix commented Jul 9, 2020

fernandez85 commented Jul 10, 2020

fernandez85 commented Oct 21, 2020 • edited Loading

bialix commented Oct 22, 2020

fernandez85 commented Jun 6, 2020 •

edited

Loading

fernandez85 commented Jun 7, 2020 •

edited

Loading

bialix commented Jun 10, 2020 •

edited

Loading

bialix commented Jun 10, 2020 •

edited

Loading

fernandez85 commented Jun 10, 2020 •

edited

Loading

fernandez85 Jun 15, 2020 •

edited

Loading

fernandez85 Jul 9, 2020 •

edited

Loading

fernandez85 commented Jul 9, 2020 •

edited

Loading

bialix commented Jul 9, 2020 •

edited

Loading

fernandez85 commented Oct 21, 2020 •

edited

Loading