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

src/client/conf-power.c: Add support for 803.2bt parameters. #449

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andreas-hofmann
Copy link
Contributor

This patch adds support for the 802.3bt parameters to lldpcli.

@vincentbernat
Copy link
Member

Thanks! This looks good. Could you add a test in tests/integration/test_dot3.py?

@bryceremick
Copy link

@andreas-hofmann Hello, what is the status of this PR? I've tested it and an issue I'm seeing is that
requested-a, requested-b, allocated-a, allocated-b, pd-load are not optional, which is contrary to what was suggested in this comment (which is what you based this PR off of)

@d1115choi
Copy link

Hi all,
I am wondering why the PR is not merged yet or what things are not resolved?

@vincentbernat
Copy link
Member

I was waiting for tests to be added. I suppose I could merge as is and add tests myself.

@andreas-hofmann
Copy link
Contributor Author

Hi @vincentbernat (and everyone else for that matter). I'm sorry for pretty much abandoning this PR - I developed this on my old job, but after some requirements changed it was not needed and due to private reasons I obviously did not get around to actually finish it off with some tests. I hope this still helped some people out there, and if you'd like to merge it, I'd of course be happy, too.
Sorry again for any inconvenience caused.

Signed-off-by: Andreas Hofmann <git@andreas-hofmann.org>
@vincentbernat
Copy link
Member

I have rebased the code against master. I'll look into adding tests.

@vincentbernat
Copy link
Member

After trying to add some tests, I discovered that 802.3bt support in lldpd was broken since the beginning. I don't know exactly why, but the TLV generated had an incorrect length. After digging a bit more, it seems Wireshark does not agree on everything present in the TLV. Maybe the support introduced in #350 was good enough to parse, but the sending part was not tested? Or maybe it's based on an earlier version of the standard? As the standard is not freely available, it is difficult for me to check. I am not comfortable maintaining something broken. The work previously done is still present in the history in case someone want to dig a bit.

@vincentbernat
Copy link
Member

I've added back the parsing part of 802.3bt.

@ShroudedNight
Copy link

In re spec availability: The 802.3bt TLV definitions appear to be included in the complete 802.3-2022 spec roll-up available gratis as part of the IEEE "GET Program" under section 79 "IEEE 802.3 Organizationally Specific Link Layer Discovery Protocol (LLDP) type, length, and value (TLV) information elements"

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.

None yet

5 participants