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

Add option to IntelHex.segments to split segments on alignment boundaries #21

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

Conversation

sTywin
Copy link

@sTywin sTywin commented Jan 24, 2018

In addition to finding contiguous occupied data addresses, it is often
useful to be able to further split these segments based on an integer
alignment, such as a flash page size or other block size. An optional
alignment parameter is added to IntelHex.segments which allows any
segments that span an integer multiple boundary of the alignment to
be split into multiple sub-segments. The semantics of the returned list
of ordered tuple objects is unchanged; that is, regardless of the given
alignment parameter, all addresses will be traversed as contiguous
segments.

I have also extended the test_segments portion to add support for testing the alignment parameter without changing any of the existing tests.

Scott Armitage added 2 commits January 24, 2018 10:12
…ries

In addition to finding contiguous occupied data addresses, it is often
useful to be able to further split these segments based on an integer
alignment, such as a flash page size or other block size. An optional
`alignment` parameter is added to IntelHex.segments which allows any
segments that span an integer multiple boundary of the alignment to
be split into multiple sub-segments. The semantics of the returned list
of ordered tuple objects is unchanged; that is, regardless of the given
alignment parameter, all addresses will be traversed as contiguous
segments.
@bialix
Copy link
Member

bialix commented Jan 25, 2018

It will be nice to have some explanation or examples in tests or in main code, or here, what you want to achieve with alignment. My understaing so far you want your segment to start right at the alignment boundary, say we have boundary 4, so segments should be 0-3, 4-7, 8-11 and so on. Is it correct?

Please, make a seprate test method (or several) for your new functionality.

Also, if possible, rewrite the following statement in simpler form - it's hard to parse it for me personnaly. Probably my Python-foo is not strong enough :-/

return [(a, b+1) for (x, y) in zip(beginings, endings) for (a, b) in _align(x, y, alignment)]

Also, it would be simpler to test and support new code if you extract inner function def _align(start, end, align): to module/class level and provide couple of tests just for it.

Ideally, I'd like to see your new tests checks cover some edge cases when segement is already aligned or not, spans several alignment blocks.

Thank you. I hope that's not too much to ask you.

@bialix
Copy link
Member

bialix commented Jan 25, 2018

Also I find it simpler if you test entire return value from segments() method, rather than 10 separate checks which hide the entire picture. I can't see a forest behind those trees :-/
Please use shorter data array for this, no need for 1Mbyte blob.

Scott Armitage added 3 commits January 25, 2018 13:30
Although this helper routine is intended for use within .segments,
it is cleaner to read and test as an independent function. We
rename it _align_segment to give it context, now that it is not
located within the segment method.
This should cover pretty much all the corner cases, plus some of the
error handling (such as bad parameters).
We construct one IntelHex object with several different types of
aligned and unaligned segments, then get the aligned segments in
one shot and check them against the expected values.
@sTywin
Copy link
Author

sTywin commented Jan 25, 2018

My understaing so far you want your segment to start right at the alignment boundary, say we have boundary 4, so segments should be 0-3, 4-7, 8-11 and so on. Is it correct?

You got it. I basically want the ability to get a list of occupied memory segments that don't span alignment boundaries, aka block boundaries or page boundaries, whatever your application may be. For me, it's flash pages on a microcontroller.

Please, make a seprate test method (or several) for your new functionality.

Done. I split out the test portions with the alignment parameter set, and created a separate test class for the helper function entirely.

Also, if possible, rewrite the following statement in simpler form - it's hard to parse it for me personnaly. Probably my Python-foo is not strong enough :-/

The only clean way I can see to re-write it is to collapse the generator expressions into actual in-memory lists. As written now, only the final list comprehension is stored in memory. We can do this if you like, but it's basically just a double loop to get the subsegments of each segment, and also to add 1 to the end value (to make it exclusive instead of inclusive):

for segment in self.segments():
    for subsegment in align(segment, alignment):
        output += [ (subsegment.start, subsegment.end+1) ]

Also, it would be simpler to test and support new code if you extract inner function def _align(start, end, align): to module/class level and provide couple of tests just for it.

Done; renamed it to _align_segment(start, end, alignment) to give it some context, and added a docstring.

Ideally, I'd like to see your new tests checks cover some edge cases when segement is already aligned or not, spans several alignment blocks.

Done. I think they are fairly comprehensive now.

Also I find it simpler if you test entire return value from segments() method, rather than 10 separate checks which hide the entire picture. I can't see a forest behind those trees :-/

I actually agree, I was just trying to use the same format as the existing test. I've re-written my test cases. I don't mind either way if you want to consolidate or split up the individual tests.

We already had left- and right-justified test cases, so it seems
trivial to add a centered test case as well, where neither the
start nor the end are aligned with the boundaries.
@bialix
Copy link
Member

bialix commented Jan 26, 2018

Just a note about principal inefficiency of segements method: it relies on addresses method which returns a full list of all addresses in IntelHex object. So, if you want to optimize - you need to start much deeper. (Hint: storage of data).

@sTywin
Copy link
Author

sTywin commented Jan 26, 2018

Sure, but I didn't set out to re-write segments, just avoid adding inefficiencies that weren't there before :) I don't feel strongly either way; I'll give you both versions. I personally find the double-loop list comprehension fairly straightforward.

That said, I was poking around and found an issue with recursion depth for long segments being split on short alignments. I'll push an update probably tomorrow that unrolls the recursion and just iterates instead.

Recursion is neat, but Python is not the best language for it. There
is no real need to use it here, so we break the recursive call out
into an interative loop instead. Only the `start` of the remainder
changes, so we increment it same as the recursive call, and once we
get to a segment that no longer needs to be further split, we
`return` (raising `StopIteration`), ending the generator.

The corresponding test was written to make the recursive approach
fall over after the default recursion depth of 1000 was hit. In
that implementation, the number of sub-segments was equal to the
eventual recursion depth. The test as-written will generate 65536
sub-segments, well beyond the recursive capabilities, but handled
fine by the iterative approach. If we make the segment upper bound
much larger, we start slowing down the tests, so this value seems
like a good compromise.
@sTywin
Copy link
Author

sTywin commented Jan 27, 2018

I've unrolled the recursion into an iteration. No noticeable change in speed. I've also added a stress-test for _align_segment that easily made the recursive approach fall over, but gives the iterative approach no problems whatsoever.

@sTywin
Copy link
Author

sTywin commented Jan 27, 2018

Here are a few different options for the final return statement.

Option 1 (current; nested list comprehension):

return [(a, b+1) for (x, y) in zip(beginings, endings) for (a, b) in _align_segment(x, y, alignment)]

Option 2 (verbose; explicit double loop with append):

subsegments = []
for (x, y) in zip(beginings, endings):
    for(a, b) in _align_segment(x, y, alignment):
        subsegments += [ (a, b+1) ]
return subsegments

Option 3 (itertools.chain):

seg_generators = [ _align_segment(x, y, alignment) for (x, y) in zip(beginings, endings) ]
return [ (a, b+1) for (a, b) in itertools.chain(*seg_generators) ]

@sTywin
Copy link
Author

sTywin commented Jan 28, 2018

Option 4: same list comprehension as option 1, but with line breaks to show double-loop format of option 2:

return [(a, b+1)
        for (x, y) in zip(beginings, endings)
            for (a, b) in _align_segment(x, y, alignment)]

@bialix
Copy link
Member

bialix commented Sep 27, 2019

Sorry for not working on your pull request. I'm looking for a new maintainer for Python IntelHex project. I hope someone will help.

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.

2 participants