Use marker types for Port read/write access

[dbeckwith]

Changes the Port struct to use a type parameter and marker types for restricting read/write access instead of using entirely separate structs, reducing some code duplication and making it easier to be generic over access types.

I'm fairly confident this is a compatible change since the A parameter on Port defaults to ReadWriteAccess and PortReadOnly and PortWriteOnly still exist, albeit as type aliases now.

This conflicts with #247 a bit but it should be easy for me to deal with the conflicts.

[dbeckwith]

Looks like this change causes some minor type inference breakage in the CI tests. I feel like in this specific case the data type should be specified anyway. Maybe this breakage is okay and I should just update the test?

[phil-opp]

Thanks a lot for the pull request!

To avoid the inference issues we could make Port::new always return a port with read/write access and add separate new_read_only() and new_write_only() constructor functions for creating ports with read-only and write-only bounds. This would be a small breaking change, but this would be fine with me.

If we want to avoid breakage completely, we could rename Port to something else (e.g. PortGeneric) and add a type alias named Port that always has read-write permissions. However, this would make the interface more complicated, so maybe a small breaking change is better here.

[dbeckwith]

I think I like the first option to have separate constructors. I'll add that and make sure the documentation is clear about which one to use.

[dbeckwith]

Actually, to stay on the theme of this change of having the Port type be generic over access, it would be nice to still have some constructor which is generic over access. If new is always read-write, maybe there's a fourth constructor called like new_with_access for the generic case?

[dbeckwith]

Hmm actually looking at it, having to do PortWriteOnly::<u8>::new_write_only seems overly verbose. I'll try out your second suggestion and see if that's cleaner.

[dbeckwith]

The way I have it now I think looks the cleanest, but it's definitely a breaking change as what was Port before is now PortReadWrite and the generic version is Port. So most existing usages need to be updated to change Port to PortReadWrite.

[josephlr]

Could we write these Access structs as pub struct ReadOnlyAccess(());? It's a little shorter.

[josephlr]

I'm not a big fan of this breaking change. I would prefer PortGeneric<T,A> which is specialized as: Port<T>, etc...

Given that users will commonly use Port and rarely use PortGeneric, keeping the smaller name makes more sense to me (and it avoids a breaking change).

I think we just need to change the names, the rest of the interface looks great to me!

[josephlr]

Thanks @dbeckwith for working on this!

[phil-opp]

Perhaps we could include the access type here as well, e.g. as an additional field? What do you think about this, @josephlr?

[dbeckwith]

The access level seems like useful info to have in the debug output, I like that idea.

[josephlr]

So I played around with ways to add an "access" field to the Debug representation, and it's a little messy: bbb0493

But it produces nice output:

PortGeneric {
    port: 80,
    size: 1,
    access: ReadWrite,
}

An alternative to this messy implementation is to just use const generics

[josephlr]

So using const generics doesn't seem too bad, I put together #254 and it looks reasonable.

The debug output looks like:

PortGeneric {
    port: 80,
    size: 1,
    read: true,
    write: true,
}

[josephlr]

I added a sealed Access trait so that we can debug print the access level.

One final question: Do we want to have Port<T> = PortGeneric<T, ReadWrtieAccess> or just have Port<T, A = ReadWriteAccess>. The only reason not to get rid of PortGeneric is the inference issues pointed out by @dbeckwith in #248 (comment). However, even with the inference issue, this change wouldn't be a breaking one (the Rust SemVer guide says this is a minor change).

Personally, I would be fine with just having Port and no PortGeneric, @phil-opp what do you think?

[phil-opp]

Personally, I would be fine with just having Port and no PortGeneric, @phil-opp what do you think?

The problem I see with using a single type is the new() method would often require a type annotation. It might not be a breaking change technically, but it's still cumbersome. So I think that I'm in favor of keeping the Port::new call non-generic.

If I could redesign this API today, I would probably choose a similar approach like I did in the volatile crate: A single generic Port type, but with a non-generic new method that uses read/write permissions. For creating read-only/write-only ports, there would be separate new_read_only and new_write_only constructors, and for generic code we could add a new_generic constructor. Unfortunately such a design would is not possible with the PortReadOnly/PortWriteOnly type aliases...

[josephlr]

The problem I see with using a single type is the new() method would often require a type annotation.

It only really needs the type annotation for the u8/u16/u32 value, not the access type, but I understand it can be disruptive even if most users are already specifying the type. If this PR looks good to you, feel free to merge it.

[phil-opp]

You're right that users that already specify a type (e.g. Port<u16>) would be fine. However, there are situations where the port type can be inferred, but not the access type. For example (the CI error linked above):

let mut port = Port::new(0xf4);
port.write(exit_code as u32);

I'm not sure how common such usage is, but I would like to avoid any breakage (even if it's technically allowed) to such a low level type because it might lead to breakage in other libraries that depend on it. So I think it's better to be careful in this case. Alternatively, we can of course do a breaking release, but I'm not sure if this is worth it.

Maybe it's a good idea to merge this with PortGeneric for now and then remove PortGeneric in the next breaking release (or slowly phase it out with a deprecated alias).

[josephlr]

Maybe it's a good idea to merge this with PortGeneric for now and then remove PortGeneric in the next breaking release (or slowly phase it out with a deprecated alias).

I think that's the most reasonable solution:

• Merge this PR as it currently is
• Next Breaking release, make Port == PortGeneric and deprecate PortGeneric
• Next next Breaking release, remove PortGeneric

I agree that Port is very low level, so we need to be very cautious. This also isn't urgent, so we can wait until we actually have something the necessitates a breaking release.

[phil-opp]

Sounds like a good plan!