Queue iterator no longer takes reference to flash

[lulf]

Apologies for just dumping this PR without discussion first, but I wanted to validate the idea anyway before raising the PR, hopefully making it clearer what API i desire.

This permits more flexible use of the iterator, for instance if needing to perform flash operations based on some error while iterating. In the current API, the flash is held exclusively by the iterator, which prevents reading and/or writing to flash (in some unrelated region) while iterating.

[diondokter]

Hi, no worries!

Interesting. I see your point. The entire API doesn't hold on to the flash for the same reason, but yeah I guess the iterator is the exception.

The only downside when getting back the flash is that it could allow the user to make changes to the flash while the iterator is still making assumptions about it. This is less the case with the rest of the API. Though I guess the cache invalidates that.

So... I think we should do this.
Let me do a review.

[lulf]

Hey, another idea that occured to me. How about being able to consume the iterator and return the flash reference?

This should ensure that the iterator isn't broken, and allows the error handling condition which is my primary reason.

[diondokter]

How is that better than just dropping the iterator? If you do that you should be able to regain access to the flash from the borrow checker.

[diondokter]

Maybe add a line of doc here to explain that you shouldn't hold on to the iterator if you're making changes to the flash it's iterating over.

[diondokter]

This is a bit weird to me... I feel like the entry should carry the flash reference the user gave in the iterator next function.

[lulf]

I'm having second thoughts about these changes. I see why you'd want that, but on the other hand it is really the same case of needing to use the flash after looking at the entry data.

Since I've been able to work around the original API for now, I'm wondering if it's best if we leave this PR in the thinking box for a while (I can close it for now), because I kind of like the original API not handing out a footgun.

[diondokter]

Yeah let's wait a bit. I'm not the one in need of these changes right now.

[lulf]

How is that better than just dropping the iterator? If you do that you should be able to regain access to the flash from the borrow checker.

Because I can't hold a &mut F flash to the flash while holding the iterator in my type that wraps the QueueIterator, so I would need go outside the scope of my wrapped type, if that make sense.

[lulf]

At the moment, my code ends up like this:

    let mut it = iter(&mut self.flash, FLASH_RANGE, &mut self.cache).await?;
    MyWrappedIterator {
        it
    }

impl MyWrappedIterator {
    async fn next(&mut self) {
        match self.it.next(..).await {
            Err(e) => {
                // No way to get the flash handle inside my type
            }
            Ok(_) => {}
        }
    }
}

[diondokter]

At the moment, my code ends up like this:

    let mut it = iter(&mut self.flash, FLASH_RANGE, &mut self.cache).await?;
    MyWrappedIterator {
        it
    }

impl MyWrappedIterator {
    async fn next(&mut self) {
        match self.it.next(..).await {
            Err(e) => {
                // No way to get the flash handle inside my type
            }
            Ok(_) => {}
        }
    }
}

Ah ok! Hmmm yeah...

Then there are more options even. For example, the flash could be given out in the E of the result next to the error. Though that would be at the cost of not being able to use the ? operator.

But I don't see how this would work when you move the flash out of the iterator in your case because your next function wouldn't support that (as is)...

I think keeping the flash separate from the iterator is a fine compromise, so your original PR can stay (with my review comments)