Closed GoogleCodeExporter closed 9 years ago
GoogleCodeExporter
commented
9 years ago Attached the first patch which will show description (if available). Only
tested with Intel SSDOriginal comment by reinlrol...@gmail.com on 15 Aug 2011 at 5:06
Attachments:
GoogleCodeExporter
commented
9 years ago Thank you for the patch. I have taken a look at it. The following issues should
be resolved before merging it into the SVN.
- the patch breaks the harddisk / ssd-control identification of the current
code. Also remaining life fields and temperatures are no longer displayed. This
features should not be removed, unless there is a good reason for it.
- if all SMART attributes are shown as sensors then this can reduce the
readability of the main GUI window. Currently there are also too many "Unkown"
attributes shown (there at least the attribute id number should be added). We
should decide if really all attributes are required / useful for a user, and if
we really want the complete SMART info as sensors. Alternatively one could use
just one aggregated SMART status sensor (or a selection of the most important
ones). In the report one could still give the full information. Showing only
relevant (vs. all that could be found) information in the main window is
important.
- the OpenHardwareMonitor.Hardware.HDD.Smart namespace should be dropped into
OpenHardwareMonitor.Hardware.HDD as most of the code in
OpenHardwareMonitor.Hardware.HDD deals already with SMART attributes anyway.
The SmartId class could be removed as far as I see (too small).
- the names of SmartAttribute and SMART.DriveAttribute should be changed, in
order to make the difference clearer. For example SMART.DriveAttribute could be
renamed to SMART.DriveAttributeValue.
- the SmartAttributeNames ressource /class should be removed and replaced with
plain strings, because in the current form it can not be edited / built in
MonoDevelop. Beside this, the exact localization system we will use has not
been implemented yet. I see no problem in using the
strings directly in the in the smartAttributeNames dictionary for now.
- source code formating: source lines should not go beyond column 80. Curly
brackets should open at the end of the last line if there is enough space
Original comment by moel.mich on 21 Aug 2011 at 5:46
GoogleCodeExporter
commented
9 years ago Hello,
according to your comments:
- I extended the implementation to show the remaining life and temperature
fields again. I don't have a harddisk with temperature information for testing
at the moment.
- The harddisk/ssd identification based on available SMART attributes is not a
good solution in my opinion. The identification based on the name of the device
will be better. I don't have several hard disks here. Are there examples (e.g.
OHM reports) with different disks available which I can use for evaluation?
Then I can finish the code for this.
- For the displaying of the SMART attributes: I added a property ShowInGui
which can be modified by a constructor. So if we don't want the attribute in
the GUI, we just have to add the parameter to the initialization of the
attribute.
- I removed the OpenHardwareMonitor.Hardware.HDD.Smart namespace. Renaming of
SMART.DriveAttribute is also done
- I removed the resource file, but kept the class SmartAttributeNames to reduce
code changes
- Code formatting is done mostly, I didn't check every line for each length.
Are there tools wich do this automatically?
Attached you will find the changes i did
Original comment by reinlrol...@gmail.com on 21 Aug 2011 at 9:14
Attachments:
GoogleCodeExporter
commented
9 years ago We thought that the harddisk / ssd identification first by SMART attribute
structure, and then by name (or parts thereof) would yield a better guess for
correct reading on unkown or rare hardware.
Alternatively one could as well do the identification by (parts of the) name.
Doing further identification by attribute in a second stage could be
complicated (above all when one tries to minimize code duplication).
A third option could to to just identify the known harddisk / ssd by (parts of
the) name and not show any SMART attributes for "unknown" devices.
About code formatting: one can configure visual studio such that the curly
brackets style used in the Open Hardware Monitor source is done automatically
(also by the auto-formatting feature). For the line length one can shown a
vertical line in the editor window at any position (column 80 in this case).
With this one can see directly when lines get too long. For Visual Studio 2010
I use the following extension:
http://visualstudiogallery.msdn.microsoft.com/0fbf2878-e678-4577-9fdb-9030389b33
8c/
Original comment by moel.mich on 23 Aug 2011 at 8:39
GoogleCodeExporter
commented
9 years ago Attached you find an updated patch with identifiaction by part of the names.
The life cycle attributes are also detected by the SMART IDs if the harddisk is
unknown. Furthermore I added additional data to the report of HDDs.
I also attached a report showing the extensions for the harddiskOriginal comment by reinlrol...@gmail.com on 27 Aug 2011 at 1:45
Attachments:
GoogleCodeExporter
commented
9 years ago Thank you for the new patch. There are still a few issues that should be fixed:
- I think it would be better to use StringComparison.InvariantCultureIgnoreCase
in the hardware library. The results of code in the hardware library should not
depend on any current culture settings.
- The license header should be edited correctly. Completely new created files
should just mention the author as "Initial Developer" and the current year. The
"Contributor" list is empty in this case. Where substantial parts of existing
files are used the, author should be added to the Contributor list and the year
updated.
- The patch implementation still does not completely reproduce all features
currently available. The remaining life attribute is not visible for example on
my system (OCZ Vertex 2 SSD).
- Some of the attribute identification is now done in the GenericHarddisc class
while others are done externally in the specialized classes like
HarddiscIndilinxSsd for example. If for example a Sandforce SSD is identified
late (that means by attribute) then it will not be an instance of
HarddiscSandforceSsd (which is not very nice). If the specialized classes like
HarddiscIndilinxSsd do not add anything beside a dictionary of attributes, then
these dictonaries could be moved as well into the GenericHarddisc class while
all other Harddisc classes are removed. The code from
GenericHarddisc.CreateInstance could then be moved into an
IdentifyHarddiskByName method and called as well from the GenericHarddisc
constructor.
- The code should be much more conservative when displaying attributes. It
should only display an attribute with its name if we really know that the name
and value is correct. SMART attributes get often "misused" for different
things, depending on the manufacturer. For SSDs for example, most of the
original labels just don't make any sense (because there is no spinning disk).
As far as I know there is no SSD with a temperature sensor. The temperature
attributes on SSDs are just for not triggering any alarm in poorly written
SMART check codes. Also any redundant data should be removed before displaying
to the user. There can be up to 3 different temperature attributes, when there
really is only one sensor. In these cases only one temperature should be
displayed.
- The report should be formatted such that rows do not exceed column 80.
- RunsOnSupportedPlattform: The reasons for using the original code (in many
places of the project) can be found here:
http://www.mono-project.com/FAQ:_Technical (How to detect the execution
platform ?)
I have attached reports created with my current system.
Original comment by moel.mich on 29 Aug 2011 at 7:45
Attachments:
GoogleCodeExporter
commented
9 years ago Hello,
according to your comments:
- I changed to InvariantCultureIgnoreCase.InvariantCultureIgnoreCase
- I changed the license header
- I changed the implementation of device identification. Can you please check
with your harddisc if you get the expected output?
- The code is not more conservative until now. I can implement checks for it
but I think the better solution is to add the missing attribute in the next step
- I changed the report to not exceed 80 columns. Makes the report less readable
in my opinion.
Additional Changes:
- I added a Win32 namespace for DeviceIoControl calls which I want to use also
for the battery monitoring
- The threshold value is also read out now
Attached you will find a patch and a report of my system
Original comment by reinlrol...@gmail.com on 30 Aug 2011 at 7:46
Attachments:
GoogleCodeExporter
commented
9 years ago I still don't get the correct output on my system. I would really want a
conservative implementation, that shows only additional fields if they are
correct (and also really preserves the current functionality). I think I will
fix that myself. I will also look how I can integrate this into the SVN
version. For the future, please don't add any refactoring of existing code into
patches if it is not absolutely required.
About the report: Yes this doesn't make the report very readable. I will see a
if a two table solution is better and fits within the 80 columns.Original comment by moel.mich on 10 Sep 2011 at 7:33
GoogleCodeExporter
commented
9 years ago This issue is closed with revision 361.Original comment by moel.mich on 31 Dec 2011 at 5:34
Original issue reported on code.google.com by
adipiciu...@gmail.comon 22 Jul 2011 at 11:34Attachments: