fix: improved error messages

[sethp]

Hi! First of all, thank you for the tool! It's been great using galette to program some GALs, and I really appreciate the work you've put into it. The "testcases" have been valuable for us both as light syntax documentation and as a great source of examples for how to solve problems with these chips!

This PR is my attempt to improve a few confusing situations I ran into the other day:

1. We have a Makefile that runs galette in a loop over multiple inputs. I modified two files at once, and spent a fair few minutes trying to figure out why e.g. line 7 was referring to an unknown pin: turns out, I was looking at the wrong file!
2. When I accidentally redefined the same output within a file, I thought the tool was telling me that I'd somehow assigned two different names to the same physical pin, rather than that I had two conflicting equations for the same output.
3. Seeing "too many product terms," I did not understand that "product term" is used to describe the inputs to the OR gate. I thought it was saying I was mis-using the AND array, somehow, despite the error describing an equation that only had ORs in it. It also took a while to figure out how many terms I needed to remove to get down underneath the limit I was exceeding.

If you'd like, I'm happy to split these up into separate PRs, but they're each pretty small so I thought I'd try them first as a batch.

Additionally, in researching the third item I think I discovered an issue when using galette with a 16v8 (possibly also the 22v10?). In the ATF16V8C datasheet, I see the following:

8.1 ATF16V8C Registered Mode
[...] Eight product terms are allocated to the sum term. For a combinatorial output or I/O, the output enable is controlled by a product term, and seven product terms are allocated to the sum term.

It's not a problem for me now, actually (since my only-sum equation was easy to turn into an only-product equation by inverting the output), and I haven't had time to test what happens if galette relaxes the bounds for registered pins (or is it all pins, if any put us in "registered mode"?). I did want to check my understanding against yours, though: do you think this is a case worth teaching galette to handle?

[simon-frankau]

Nice! My original goal was to keep the error messages as similar as possible to galasm, but "better error messages" seems preferable to unnecessary backwards compatibility. This is definitely an improvement.

My preference is to split out the max terms issue into a separate issue, since then we can get this submitted quickly and I can spend a little longer to swap in my memory of "ATF16V8C Registered Mode" etc. If I understand you, and my decayed memory is correct, I think it's a case worth handling.

Thank you for these improvements.

[simon-frankau]

I'm no Rust style expert, but I'm wondering if we can make this just a tiny bit clearer that nothing interesting is being dropped by the "_" in this closure. Maybe .set_base should return an Option and .ok_or is applied, or there's a pattern match against a "()" or something. Most explicit might be to make AlreadyDefined a single-element enum, but that seems a little heavy-handed.

Any opinions?

[sethp]

I'm no expert either, but I did find at least one style guide that did prohibit () as a result type:

Never use () as an error type, even where there is no useful additional information for the error to carry.

(and then goes on to list the traits () doesn't implement, making it awkward to use in the E position).

The suggestion is to implement a full enum, but that does feel like a lot in this case.

I like your suggestion of Option's ok_or, but Option<()> felt a little weird to me: it's either some nothing or nothing! But the only other idea I have, uh, does not pass the "not-heavy-handed" bar:

pub enum Success {
    Succeeded,
    Failed,
}

impl Success {
    pub fn ok_or_else<E, F>(self, err: F) -> Result<(), E>
    where
        F: FnOnce() -> E,
    {
        use Success::*;

        match self {
            Succeeded => Ok(()),
            Failed => Err(err()),
        }
    }
}

// ...

impl OLMC {
    pub fn set_base(&mut self, pin: &Pin, term: Term, pin_mode: PinMode) -> Success {
        // ...
    }
    // ...
}

I guess I'm not the first person to have this idea, but it's not a particularly popular one either.

I think I'll switch it to Option<()>?

[simon-frankau]

Thanks for fixing the "multible" typo!

[simon-frankau]

Thanks, this looks good to me. Sorry about the slow turn around - other distractions.