This PR contains some fixes to correctly parse EDID 1.4 structures, mostly for the Basic Display Parameters section.
I've also removed the manufacturing date from all EDID lists as EDID 1.4 allows to specify a model year instead of it and I didn't see a beautiful way of integrating it into the tables without cluttering them too much. Instead, I've added a General Information table to the EDID detail page which contains the EDID version, the maximum resolution, the video input type and either the model year or the date of manufacturing.
This is what it looks like now (I've marked all fields that were added in EDID 1.4):
The root cause here is probably that I am rather unhappy with the way the parser was implemented. While this would introduce tighter coupling, I believe it wouldn't hurt to have the parser directly populate the model. This would save us a bunch of code and having declared properties feels cleaner than stuffing everything into a dict first. Then again, my experience with Python is rather limited and what I am saying might be completely stupid. Also, this would mean a major refactor of the parser.
Note 2
In https://github.com/timvideos/edid.tv/pull/20, I've previously introduced a __future__ import to models.py to get Python 3 division behavior as this was the only place where division is used in that file anyway. Here in this PR, I've opted to rely on Python 2 division behavior at one place as the same file already uses that. This makes the code somewhat inconsistent. The code base really should be upgraded to Python 3.
Note 3
I never really tried clicking the "Update" button on EDID detail page before (I kinda thought it would be just the same as in the Django admin backend). I just realized that I would have needed to update the edit forms in https://github.com/timvideos/edid.tv/pull/18. This PR here is also missing the necessary changes. I suggest leaving it like this for now and fixing the forms in another PR.
mithro
commented
5 years ago
This PR contains some fixes to correctly parse EDID 1.4 structures, mostly for the Basic Display Parameters section.
I've also removed the manufacturing date from all EDID lists as EDID 1.4 allows to specify a model year instead of it and I didn't see a beautiful way of integrating it into the tables without cluttering them too much. Instead, I've added a General Information table to the EDID detail page which contains the EDID version, the maximum resolution, the video input type and either the model year or the date of manufacturing.
This is what it looks like now (I've marked all fields that were added in EDID 1.4):
Note
I am not really happy with the fix in https://github.com/timvideos/edid.tv/commit/30e5db2ec899289f939be7d6b5d2ca471e4d1545. The issue here is that
EDIDParsingTestCaseruns the EDID parser with EDID version 1.3 and thentest_version_revision_valid()tries to populate the EDID model from the parsed data as if it was an EDID 1.4. https://github.com/timvideos/edid.tv/commit/a0660e370f5f802d3f60897c54423773fb1db953 has introduced branches depending on the EDID version in_populate_from_edid_parser()and expects that EDID 1.4-specific values likeColor_Bit_Depthwould be set which they are not because we previously parsed the EDID with version 1.3.The root cause here is probably that I am rather unhappy with the way the parser was implemented. While this would introduce tighter coupling, I believe it wouldn't hurt to have the parser directly populate the model. This would save us a bunch of code and having declared properties feels cleaner than stuffing everything into a dict first. Then again, my experience with Python is rather limited and what I am saying might be completely stupid. Also, this would mean a major refactor of the parser.
Note 2
In https://github.com/timvideos/edid.tv/pull/20, I've previously introduced a
__future__import tomodels.pyto get Python 3 division behavior as this was the only place where division is used in that file anyway. Here in this PR, I've opted to rely on Python 2 division behavior at one place as the same file already uses that. This makes the code somewhat inconsistent. The code base really should be upgraded to Python 3.Note 3
I never really tried clicking the "Update" button on EDID detail page before (I kinda thought it would be just the same as in the Django admin backend). I just realized that I would have needed to update the edit forms in https://github.com/timvideos/edid.tv/pull/18. This PR here is also missing the necessary changes. I suggest leaving it like this for now and fixing the forms in another PR.